fix(pi-session): a failed remote mine reported success — MCP escapes the payload
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.
This commit is contained in:
Joakim Persson
+205
-37
@@ -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: <title> | <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,6 +799,10 @@ for path in paths:
|
||||
pass
|
||||
|
||||
exported += 1
|
||||
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
|
||||
@@ -626,7 +810,7 @@ for path in paths:
|
||||
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"
|
||||
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 ""
|
||||
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
|
||||
|
||||
Reference in New Issue
Block a user