From 6e1f4f30fc96d27ce410e1d78e46a3de199c4cc7 Mon Sep 17 00:00:00 2001 From: Joakim Persson Date: Sun, 16 Aug 2026 00:28:50 +0200 Subject: [PATCH] =?UTF-8?q?fix(pi-session):=20a=20failed=20remote=20mine?= =?UTF-8?q?=20reported=20success=20=E2=80=94=20MCP=20escapes=20the=20paylo?= =?UTF-8?q?ad?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The remote-mine leg decided success with `'"error"' in body`. MCP answers a hard tool failure with HTTP 200 and a JSON-RPC *result* whose content[].text carries the tool's own JSON as an ESCAPED string, so those bytes are \"error\" and the substring never matches. On 2026-08-15 (EMB-7KJ4VR4G, first boot of the fresh pi-devbox image) a mine that failed with {"success": false, "error": "source directory not found: '/data/feed/emb-7kj4vr4g'"} printed "Done. Wing 'wing_conversations' updated." directly under that error and exited 0. Transcripts had been rsynced for the whole session and filed nowhere; the only artifact anyone would check said it worked. - classify(): parse the envelope instead of grepping it. Catches JSON-RPC errors, MCP isError, and inner success=false/error, and distinguishes "verified ok" from "unverified: no JSON tool payload" rather than assuming. - --self-test: six recorded MCP responses (fixture 1 is the real 2026-08-15 body) plus a regression guard asserting the old substring check is blind to it. Needs no palace, no network, no sessions dir. - Preflight warning when the rsync destination path and MEMPALACE_PI_REMOTE_PATH disagree. The /data/feed default assumes a CONTAINERIZED palace server; a native one (systemd unit / uv tool) sees host paths, and then the two must match. Warned in preflight so --dry-run and --prepare surface it too. - Remote mode no longer previews NEW/SKIP from the LOCAL palace: dedup happens on the palace host keyed on the remote inbox path, so this machine cannot answer it. Tags become [?] and the summary says who decides. It had been reporting "6 already filed" about a palace it was not feeding. --- bin/mempalace-pi-session | 256 ++++++++++++++++++++++++++++++++------- 1 file changed, 212 insertions(+), 44 deletions(-) diff --git a/bin/mempalace-pi-session b/bin/mempalace-pi-session index 1b51286..177a55e 100755 --- a/bin/mempalace-pi-session +++ b/bin/mempalace-pi-session @@ -43,6 +43,16 @@ # MEMPALACE_PI_SSH_TARGET (where to rsync) and # MEMPALACE_PI_REMOTE_PATH (what that inbox is called server-side). # +# MEMPALACE_PI_REMOTE_PATH is the path AS THE SERVER PROCESS SEES +# IT, and the default (/data/feed) assumes a CONTAINERIZED server +# with the inbox bind-mounted there. A NATIVE server (systemd unit / +# uv tool / plain `mempalace serve`) sees host paths, so there it +# must equal the path half of MEMPALACE_PI_SSH_TARGET. Get this +# wrong and rsync still succeeds while the mine fails with +# "source directory not found" — so a mismatch between the two is +# warned about at ship time, and the mine's own failure is now +# detected properly (see classify() in run_remote_mine). +# # Labelling: every exported transcript begins with a synthetic header # [session: | <cwd> | <YYYY-MM-DD> | source: pi] # so post-mine search results are self-identifying (pi vs opencode vs other). @@ -123,6 +133,7 @@ MIN_ASSISTANT_CHARS=1000 DRY_RUN=0 DO_REPAIR=0 PREPARE_ONLY=0 +SELF_TEST=0 MODE="auto" REASON="" PI_SESSIONS_DIR="${PI_SESSIONS_DIR:-$HOME/.pi/agent/sessions}" @@ -199,7 +210,12 @@ Options: container-start). Useful when triggers log to a file. --dry-run Export + list; do not mine into palace. Each session is tagged [NEW] or [SKIP] based on whether its - source_file is already in the palace. + source_file is already in the palace. In remote mode + the tag is [?]: dedup is decided by the palace host, + which this machine's local palace copy cannot answer. + --self-test Run the remote-mine response classifier against + recorded MCP responses and exit. Needs no palace, no + network and no sessions dir. --repair Run `mempalace repair` after mining (opt-in). WARNING: repair does a destructive in-place HNSW rebuild. If it races a live MCP connection or @@ -252,6 +268,140 @@ Rationale: EOF } +# ── Remote mine over MCP ───────────────────────────────────────────── +# Usage: run_remote_mine <url> <token> <source> <wing> <agent> +# run_remote_mine --self-test +# +# WHY THIS IS A FUNCTION WITH A SELF-TEST: MCP answers a hard tool failure with +# HTTP 200 and a JSON-RPC *result* whose content[].text holds the tool's own +# JSON as an ESCAPED STRING. This code used to decide success with +# `'"error"' in body`, which can never match those bytes (they are \"error\"), +# so on 2026-08-15 a mine that failed with +# {"success": false, "error": "source directory not found: '/data/feed/...'"} +# was reported as "Done. Wing updated." and nothing was filed. A silent +# false success in a feeder is worse than a crash: the only artifact says it +# worked. The fixtures below pin that exact body so it cannot come back. +run_remote_mine() { + python3 - "$@" <<'PY' +import json, sys, urllib.error, urllib.request + + +def classify(body): + """Return (ok, note) for an MCP tools/call response body. + + ok=False means the mine demonstrably failed. note carries the reason, or — + when ok is True — an "unverified" caveat if the response contained no JSON + tool payload to adjudicate. Never claim more than the bytes support. + """ + try: + env = json.loads(body) + except ValueError: + return False, "response was not JSON: " + body[:200].replace("\n", " ") + if not isinstance(env, dict): + return False, "response was not a JSON object" + if env.get("error") is not None: # JSON-RPC transport-level error + return False, "JSON-RPC error: " + json.dumps(env["error"])[:300] + result = env.get("result") + if not isinstance(result, dict): + return False, "response carried no result object" + if result.get("isError"): + return False, "MCP isError set: " + json.dumps(result.get("content"))[:300] + saw_payload = False + for item in result.get("content") or []: + text = item.get("text") if isinstance(item, dict) else None + if not isinstance(text, str): + continue + try: + payload = json.loads(text) # the escaped inner JSON + except ValueError: + continue # plain prose content: nothing to judge + if not isinstance(payload, dict): + continue + saw_payload = True + if payload.get("success") is False: + return False, str(payload.get("error") or "tool reported success=false") + if payload.get("error"): + return False, str(payload["error"]) + if not saw_payload: + return True, "unverified: no JSON tool payload in the response" + return True, "" + + +FIXTURES = [ + # 1. The real 2026-08-15 failure: HTTP 200, JSON-RPC result, tool failed. + ('{"jsonrpc": "2.0", "id": 1, "result": {"content": [{"type": "text", ' + '"text": "{\\n \\"success\\": false,\\n \\"error\\": \\"source directory ' + 'not found: \'/data/feed/emb-7kj4vr4g\'\\"\\n}"}]}}', False), + # 2. A real success. + ('{"jsonrpc":"2.0","id":1,"result":{"content":[{"type":"text","text":' + '"{\\"success\\": true, \\"mode\\": \\"convos\\", \\"output\\": \\"Drawers filed: 12\\"}"}]}}', True), + # 3. JSON-RPC level error (bad method, auth rejected at protocol level). + ('{"jsonrpc":"2.0","id":1,"error":{"code":-32601,"message":"Method not found"}}', False), + # 4. MCP tool-level isError flag. + ('{"jsonrpc":"2.0","id":1,"result":{"isError":true,"content":[{"type":"text","text":"boom"}]}}', False), + # 5. Not JSON at all (proxy error page, 502 HTML). + ('<html><body>502 Bad Gateway</body></html>', False), + # 6. Success-shaped envelope with prose content: cannot be called a failure. + ('{"jsonrpc":"2.0","id":1,"result":{"content":[{"type":"text","text":"mined 3 files"}]}}', True), +] + +args = sys.argv[1:] + +if args[:1] == ["--self-test"]: + failures = 0 + for i, (body, want_ok) in enumerate(FIXTURES, 1): + got_ok, note = classify(body) + if got_ok != want_ok: + failures += 1 + print(f" [{'ok ' if got_ok == want_ok else 'FAIL'}] fixture {i}: " + f"want_ok={want_ok} got_ok={got_ok} note={note[:70]!r}") + # Regression guard: the detector this replaced must be shown blind to #1. + old_detector_sees_it = '"error"' in FIXTURES[0][0] + if old_detector_sees_it: + failures += 1 + print(f" [{'ok ' if not old_detector_sees_it else 'FAIL'}] regression guard: " + f"substring detector sees fixture 1? {old_detector_sees_it} (must be False)") + print("SELF-TEST " + ("FAILED" if failures else "PASSED")) + sys.exit(1 if failures else 0) + +url, token, source, wing, agent = args[:5] +payload = json.dumps({ + "jsonrpc": "2.0", + "id": 1, + "method": "tools/call", + "params": { + "name": "mempalace_mine", + "arguments": {"source": source, "mode": "convos", "wing": wing, "agent": agent}, + }, +}).encode() +headers = {"Content-Type": "application/json", "Accept": "application/json"} +if token: + headers["Authorization"] = f"Bearer {token}" +try: + with urllib.request.urlopen( + urllib.request.Request(url, data=payload, headers=headers), timeout=900 + ) as resp: + body = resp.read().decode("utf-8", "replace") +except urllib.error.HTTPError as exc: + detail = exc.read().decode("utf-8", "replace")[:500] if hasattr(exc, "read") else "" + print(f"error: remote mine transport failed: HTTP {exc.code} {exc.reason} {detail}", + file=sys.stderr) + sys.exit(1) +except urllib.error.URLError as exc: + print(f"error: remote mine transport failed: {exc}", file=sys.stderr) + sys.exit(1) + +print(body[:4000]) +ok, note = classify(body) +if not ok: + print(f"error: remote mine reported failure: {note}", file=sys.stderr) + sys.exit(1) +if note: + print(f"warning: remote mine {note}", file=sys.stderr) +sys.exit(0) +PY +} + # ── Parse args ─────────────────────────────────────────────────────── while [[ $# -gt 0 ]]; do case "$1" in @@ -268,6 +418,7 @@ while [[ $# -gt 0 ]]; do --agent) AGENT="${2:-}"; shift 2 ;; --sessions-dir) PI_SESSIONS_DIR="${2:-}"; shift 2 ;; --dry-run) DRY_RUN=1; shift ;; + --self-test) SELF_TEST=1; shift ;; --repair) DO_REPAIR=1; shift ;; --no-repair) shift ;; # deprecated alias; no-repair is the default --) shift; break ;; @@ -276,6 +427,14 @@ while [[ $# -gt 0 ]]; do esac done +# The classifier self-test is pure logic: no palace, no network, no sessions +# dir. Dispatch before preflight so it runs anywhere, including in CI. +if [[ $SELF_TEST -eq 1 ]]; then + echo "mempalace-pi-session --self-test: remote-mine response classifier" + run_remote_mine --self-test + exit $? +fi + # ── Preflight ──────────────────────────────────────────────────────── if [[ ! -d "$PI_SESSIONS_DIR" ]]; then echo "error: pi sessions dir not found at $PI_SESSIONS_DIR" >&2 @@ -303,6 +462,22 @@ if [[ "$MODE" == "remote" ]]; then if [[ -z "$SSH_CONFIG" && -f "$HOME/.ssh-local/config" ]]; then SSH_CONFIG="$HOME/.ssh-local/config" fi + # Remote mode names the same inbox twice: where rsync PUTS the files, and + # what the SERVER is told to mine. They may legitimately differ + # (containerized server: host dir bind-mounted elsewhere), but when they + # differ by accident rsync still succeeds and only the mine fails — the + # 2026-08-15 /data/feed incident, where transcripts shipped for hours and + # were filed nowhere. Warn in PREFLIGHT so --dry-run and --prepare see it + # too, not just a full run that gets as far as shipping. + SHIP_PATH="$SSH_TARGET" + [[ "$SHIP_PATH" == *:* ]] && SHIP_PATH="${SHIP_PATH##*:}" + if [[ "${SHIP_PATH%/}" != "${REMOTE_PATH%/}" ]]; then + echo "note: shipping to '${SHIP_PATH%/}/$DEVICE' but asking the server to mine" + echo " '${REMOTE_PATH%/}/$DEVICE'. Correct only if the palace server sees" + echo " '${SHIP_PATH%/}' at '${REMOTE_PATH%/}' (containerized server with a bind" + echo " mount). A NATIVE server (systemd unit / uv tool) sees host paths —" + echo " then set MEMPALACE_PI_REMOTE_PATH='${SHIP_PATH%/}'." + fi fi for _n in MIN_MESSAGES MIN_ASSISTANT_CHARS; do if ! [[ "${!_n}" =~ ^[0-9]+$ ]]; then @@ -335,12 +510,12 @@ fi # Also classifies each export as NEW/ALREADY FILED (by source_file lookup) # so --dry-run reports the real mine-set size. Classification is advisory; # `mempalace mine --mode convos` is still the authoritative dedup. -export_count=$(python3 - "$PI_SESSIONS_DIR" "$STAGE" "$SESSION_ID" "$SINCE" "$MIN_MESSAGES" "$MIN_ASSISTANT_CHARS" <<'PY' +export_count=$(python3 - "$PI_SESSIONS_DIR" "$STAGE" "$SESSION_ID" "$SINCE" "$MIN_MESSAGES" "$MIN_ASSISTANT_CHARS" "$MODE" <<'PY' import json, os, sqlite3, sys from datetime import datetime, timezone from pathlib import Path -sessions_dir, stage, session_filter, since, min_messages, min_assistant_chars = sys.argv[1:7] +sessions_dir, stage, session_filter, since, min_messages, min_assistant_chars, mode = sys.argv[1:8] min_messages = int(min_messages) min_assistant_chars = int(min_assistant_chars) stage = Path(stage) @@ -356,7 +531,12 @@ if since: sys.exit(1) # ── Load palace's already-filed source_files (best-effort, read-only) ── -already_filed = set() +# already_filed is None in remote mode: unknown, not empty. The dedup that +# matters happens on the PALACE HOST and is keyed on the REMOTE inbox path, so +# this machine's local palace file cannot answer the question — and answering +# it anyway is how a preview comes to say "6 already filed" about a palace it +# is not feeding (2026-08-15). An honest "[?]" beats a confident wrong number. +already_filed = None if mode == "remote" else set() # Mirror mempalace's own resolution order (config.py): MEMPALACE_PALACE_PATH, # then the legacy MEMPAL_PALACE_PATH, then the default. NOT "MEMPALACE_PATH" — # that name is not a mempalace concept, and reading it silently degraded this @@ -367,7 +547,7 @@ palace_path = ( or os.path.expanduser("~/.mempalace/palace") ) chroma_db = Path(palace_path) / "chroma.sqlite3" -if chroma_db.is_file(): +if already_filed is not None and chroma_db.is_file(): try: pcon = sqlite3.connect(f"file:{chroma_db}?mode=ro", uri=True) for (sf,) in pcon.execute( @@ -619,14 +799,18 @@ for path in paths: pass exported += 1 - is_filed = str(out_path) in already_filed - if is_filed: - skipped_already_filed += 1 - status = "SKIP" if is_filed else "NEW " + if already_filed is None: + is_filed = False # unknowable here; the palace host decides + status = "? " + else: + is_filed = str(out_path) in already_filed + if is_filed: + skipped_already_filed += 1 + status = "SKIP" if is_filed else "NEW " print(f" [{status}] {out_path.name} ({turn_count} turns)", file=sys.stderr) print(f"EXPORTED {exported}") -print(f"ALREADY_FILED {skipped_already_filed}") +print(f"ALREADY_FILED {-1 if already_filed is None else skipped_already_filed}") if skipped_short: print(f"SKIPPED_SHORT {skipped_short}", file=sys.stderr) if skipped_quiet: @@ -641,7 +825,8 @@ count="$(printf '%s\n' "$export_count" | awk '/^EXPORTED / { print $2 }')" count="${count:-0}" already_filed="$(printf '%s\n' "$export_count" | awk '/^ALREADY_FILED / { print $2 }')" already_filed="${already_filed:-0}" -to_file=$(( count - already_filed )) +# -1 means "unknown" (remote mode), so guard the arithmetic. +if [[ "$already_filed" -lt 0 ]]; then to_file="$count"; else to_file=$(( count - already_filed )); fi if [[ "$count" -eq 0 ]]; then echo "no sessions qualified for export" @@ -650,15 +835,25 @@ fi echo "" echo "Exported $count session(s) to $STAGE" -echo " $to_file new → will be filed on mine" -echo " $already_filed already filed → will be skipped (dedup by source_file)" +if [[ "$already_filed" -lt 0 ]]; then + # Remote mode: dedup lives on the palace host, keyed on the remote inbox + # path. Do not translate "unknown" into a number. + to_file="$count" + echo " all $count shipped → the palace host dedups by source_file (remote mode:" + echo " this machine cannot preview what it already holds)" +else + echo " $to_file new → will be filed on mine" + echo " $already_filed already filed → will be skipped (dedup by source_file)" +fi if [[ $DRY_RUN -eq 1 ]]; then - if [[ "$to_file" -eq 0 ]]; then - echo "" + echo "" + if [[ "$already_filed" -lt 0 ]]; then + echo "--dry-run: skipping ship+mine. A real run would ship $count session(s) to" + echo " ${SSH_TARGET%/}/$DEVICE/ and let the palace host dedup them." + elif [[ "$to_file" -eq 0 ]]; then echo "--dry-run: no new sessions to mine. A real run would skip all $count." else - echo "" echo "--dry-run: skipping mine step. A real run would file $to_file new session(s)." fi exit 0 @@ -697,34 +892,7 @@ fi echo "" echo "Mining into wing '$WING'..." if [[ "$MODE" == "remote" ]]; then - if ! python3 - "$REMOTE_URL" "$REMOTE_TOKEN" "$MINE_SOURCE" "$WING" "$AGENT" <<'PY' -import json, sys, urllib.error, urllib.request - -url, token, source, wing, agent = sys.argv[1:6] -payload = json.dumps({ - "jsonrpc": "2.0", - "id": 1, - "method": "tools/call", - "params": { - "name": "mempalace_mine", - "arguments": {"source": source, "mode": "convos", "wing": wing, "agent": agent}, - }, -}).encode() -headers = {"Content-Type": "application/json", "Accept": "application/json"} -if token: - headers["Authorization"] = f"Bearer {token}" -try: - with urllib.request.urlopen( - urllib.request.Request(url, data=payload, headers=headers), timeout=900 - ) as resp: - body = resp.read().decode("utf-8", "replace") -except urllib.error.URLError as exc: - print(f"error: remote mine transport failed: {exc}", file=sys.stderr) - sys.exit(1) -print(body[:4000]) -sys.exit(1 if '"error"' in body else 0) -PY - then + if ! run_remote_mine "$REMOTE_URL" "$REMOTE_TOKEN" "$MINE_SOURCE" "$WING" "$AGENT"; then echo "error: remote mine failed" >&2 exit 5 fi