review: address pi tool-result hook review findings

Cross-model review (Claude + Codex adversarial) flagged 4 issues that
would either lose user data on Windows or silently degrade episodic
memory quality.

1. install.ps1:157-159 — DATA LOSS on Windows re-install.
   `Remove-Item -LiteralPath $skillsDst -Recurse -Force` on PowerShell
   5.1 (default Windows shell) traverses INTO a symlink target and
   deletes its contents before removing the link. A second
   `install.ps1 pi` run would wipe .agent/skills/. Detect ReparsePoint
   via Get-Item.Attributes BEFORE Remove-Item; use
   .NET Directory.Delete($path, false) on links so only the link is
   removed, never the target.

2. adapters/pi/memory-hook.ts — no subprocess timeout, hangs Pi.
   `await runHook(payload)` is awaited inside Pi's tool_result handler;
   a stuck Python child blocks Pi's event loop forever. Add 3s default
   timeout (overridable via $AGENT_HOOK_TIMEOUT_MS), kill the child on
   timeout, surface `timeout` as a separate result kind.

3. adapters/pi/memory-hook.ts — stderr was dropped, failures
   undiagnosable. Switch stdio to capture stderr (bounded to 4KB to
   avoid memory blowup on a wedged hook) and surface the first line
   in the failure notification.

4. .agent/harness/hooks/pi_post_tool.py — Pi sends tool_input with
   camelCase keys (filePath, oldString, newString), but the shared
   cc.* helpers (action_label, reflection, importance) expect Claude
   Code's snake_case keys. Without normalization, every Edit/Write
   logged by Pi degraded to "edit: ?" / "Edited ?" with empty detail.
   Add a Pi→canonical input key map applied in _normalize_input().

5. .agent/harness/hooks/pi_post_tool.py — fail-open on malformed
   payload was logging bogus "Unknown success" entries (Codex's High
   #1). If Pi ever changes the event shape or sends invalid JSON,
   episodic memory got polluted with noise instead of a real signal.
   Add a _emit_malformed() path that records an explicit
   `hook:malformed_payload` failure entry with a 200-char excerpt of
   the offending payload — visible in AGENT_LEARNINGS.jsonl as a real
   error, not noise.

Smoke-tested:
- empty payload → `hook:malformed_payload | empty payload`
- malformed JSON → `hook:malformed_payload | json decode error: ...`
- Pi camelCase Edit (filePath/oldString/newString) → produces correct
  `edit: /tmp/x.txt` action label and `Edited /tmp/x.txt: replaced 'a'
  with 'b'` reflection (was: `edit: ?` / `Edited ?`)
- well-formed bash success → unchanged behavior

Codex's concurrent-write concern (Codex #3) and the rsync-style sync
for .pi/skills (Codex #5) are NOT addressed here — they apply to
pre-existing infrastructure (post_execution.py write semantics, pi's
existing symlink path) and are scoped as separate follow-ups.

Author: codejunkie99

.agent/harness/hooks/pi_post_tool.py

@@ -33,15 +33,55 @@
     "ls": "LS",
     "task": "Task",
     "todowrite": "TodoWrite",
+    "todo_write": "TodoWrite",
     "webfetch": "WebFetch",
+    "web_fetch": "WebFetch",
+}
+
+# Pi tends to use camelCase for tool_input keys; the shared
+# claude_code_post_tool helpers (cc._action_label, cc._reflection,
+# cc._importance) expect the snake_case keys Claude Code sends.
+# Normalize a known set so action labels and reflections come out
+# meaningful instead of degrading to "edit: ?" / "Edited ?".
+_PI_INPUT_KEY_MAP = {
+    "filePath": "file_path",
+    "filepath": "file_path",
+    "path": "file_path",
+    "oldString": "old_string",
+    "oldstring": "old_string",
+    "newString": "new_string",
+    "newstring": "new_string",
+    "content": "content",
+    "command": "command",
+    "todos": "todos",
+    "todo": "todos",
+    "url": "url",
+    "pattern": "pattern",
+    "query": "query",
 }
 
 
 def _tool_name(name: str) -> str:
     if not isinstance(name, str):
         return "Unknown"
     lowered = name.strip().lower()
-    return _PI_TO_CANONICAL.get(lowered, name[:1].upper() + name[1:])
+    if lowered in _PI_TO_CANONICAL:
+        return _PI_TO_CANONICAL[lowered]
+    if "_" in name:
+        return "".join(part[:1].upper() + part[1:] for part in name.split("_") if part)
+    return name[:1].upper() + name[1:]
+
+
+def _normalize_input(raw) -> dict:
+    """Map Pi tool_input keys to the snake_case shape cc.* helpers expect."""
+    if not isinstance(raw, dict):
+        if raw is None:
+            return {}
+        return {"raw": str(raw)}
+    out: dict = {}
+    for k, v in raw.items():
+        out[_PI_INPUT_KEY_MAP.get(k, k)] = v
+    return out
 
 
 def _extract_text(content) -> str:
@@ -97,17 +137,51 @@ def _normalize_response(event: dict) -> dict:
     return resp
 
 
+def _emit_malformed(reason: str, raw_excerpt: str) -> None:
+    """Record an explicit failure entry instead of silently logging a bogus
+    'Unknown success'. If Pi changes payload shape or sends malformed
+    JSON, this surfaces in AGENT_LEARNINGS.jsonl as a real signal.
+    """
+    excerpt = raw_excerpt[:200] if isinstance(raw_excerpt, str) else ""
+    on_failure(
+        skill_name="pi",
+        action="hook:malformed_payload",
+        error=f"pi tool_result payload malformed: {reason}",
+        context=excerpt,
+        confidence=0.95,
+        importance="medium",
+        pain_score=2,
+    )
+
+
 def main() -> None:
+    raw = ""
     try:
         raw = sys.stdin.read()
-        payload = json.loads(raw) if raw.strip() else {}
-    except (json.JSONDecodeError, OSError):
-        payload = {}
+    except OSError as e:
+        _emit_malformed(f"stdin read failed: {e}", "")
+        return
+
+    if not raw or not raw.strip():
+        _emit_malformed("empty payload", "")
+        return
+
+    try:
+        payload = json.loads(raw)
+    except json.JSONDecodeError as e:
+        _emit_malformed(f"json decode error: {e.msg}", raw)
+        return
+
+    if not isinstance(payload, dict):
+        _emit_malformed(f"payload is {type(payload).__name__}, expected object", raw)
+        return
+
+    if "tool_name" not in payload:
+        _emit_malformed("missing tool_name", raw)
+        return
 
     tool_name = _tool_name(payload.get("tool_name") or "Unknown")
-    tool_input = payload.get("tool_input") or {}
-    if not isinstance(tool_input, dict):
-        tool_input = {"raw": str(tool_input)}
+    tool_input = _normalize_input(payload.get("tool_input"))
     tool_response = _normalize_response(payload)
 
     success = cc._is_success(tool_name, tool_input, tool_response)

adapters/pi/memory-hook.ts

@@ -14,15 +14,31 @@ const HOOK_SCRIPT = path.join(
   "pi_post_tool.py",
 );
 
+// Timeout for the Python child. If the hook hangs (bad import, stuck I/O),
+// Pi's tool_result handler stays blocked because the extension awaits
+// runHook(). Override via $AGENT_HOOK_TIMEOUT_MS for slow machines.
+const HOOK_TIMEOUT_MS = (() => {
+  const raw = process.env.AGENT_HOOK_TIMEOUT_MS?.trim();
+  const n = raw ? Number.parseInt(raw, 10) : NaN;
+  return Number.isFinite(n) && n > 0 ? n : 3000;
+})();
+
 let warnedMissingHook = false;
 let warnedMissingPython = false;
 let warnedHookFailure = false;
+let warnedHookTimeout = false;
 
 type PythonCandidate = {
   command: string;
   args: string[];
 };
 
+type HookResult =
+  | { kind: "ok" }
+  | { kind: "spawn-error" }
+  | { kind: "hook-failure"; stderr: string; exitCode: number | null }
+  | { kind: "timeout" };
+
 function pythonCandidates(): PythonCandidate[] {
   const envPy = process.env.AGENT_PYTHON?.trim();
   const out: PythonCandidate[] = [];
@@ -36,32 +52,77 @@ function pythonCandidates(): PythonCandidate[] {
 function tryRun(
   candidate: PythonCandidate,
   payload: Record<string, unknown>,
-): Promise<"ok" | "spawn-error" | "hook-failure"> {
+): Promise<HookResult> {
   return new Promise((resolve) => {
+    let settled = false;
+    const settle = (r: HookResult) => {
+      if (settled) return;
+      settled = true;
+      resolve(r);
+    };
+
+    let stderrBuf = "";
     const child = spawn(
       candidate.command,
       [...candidate.args, HOOK_SCRIPT],
       {
         cwd: PROJECT_ROOT,
-        stdio: ["pipe", "ignore", "ignore"],
+        // Capture stderr so a "hook-failure" notification can include
+        // the actual error instead of being undiagnosable.
+        stdio: ["pipe", "ignore", "pipe"],
       },
     );
-    child.on("error", () => resolve("spawn-error"));
+
+    const timer = setTimeout(() => {
+      try { child.kill("SIGKILL"); } catch { /* already dead */ }
+      settle({ kind: "timeout" });
+    }, HOOK_TIMEOUT_MS);
+
+    child.on("error", () => {
+      clearTimeout(timer);
+      settle({ kind: "spawn-error" });
+    });
     child.on("spawn", () => {
-      child.stdin.end(JSON.stringify(payload));
+      try {
+        child.stdin.end(JSON.stringify(payload));
+      } catch {
+        // stdin closed before we could write — handled by close/error
+      }
     });
+    if (child.stderr) {
+      child.stderr.setEncoding("utf8");
+      child.stderr.on("data", (chunk: string) => {
+        // bound stderr buffer to avoid memory blowup on a wedged hook
+        if (stderrBuf.length < 4096) stderrBuf += chunk;
+      });
+    }
     child.on("close", (code) => {
-      resolve(code === 0 ? "ok" : "hook-failure");
+      clearTimeout(timer);
+      if (code === 0) {
+        settle({ kind: "ok" });
+      } else {
+        settle({ kind: "hook-failure", stderr: stderrBuf.trim(), exitCode: code });
+      }
     });
   });
 }
 
-async function runHook(payload: Record<string, unknown>) {
+async function runHook(
+  payload: Record<string, unknown>,
+): Promise<
+  | "ok"
+  | "missing-hook"
+  | "missing-python"
+  | "timeout"
+  | { kind: "hook-failure"; stderr: string; exitCode: number | null }
+> {
   if (!existsSync(HOOK_SCRIPT)) return "missing-hook";
   for (const candidate of pythonCandidates()) {
     const result = await tryRun(candidate, payload);
-    if (result === "ok") return "ok";
-    if (result === "hook-failure") return "hook-failure";
+    if (result.kind === "ok") return "ok";
+    if (result.kind === "timeout") return "timeout";
+    if (result.kind === "hook-failure") return result;
+    // spawn-error → try the next python candidate
   }
   return "missing-python";
 }
@@ -90,18 +151,30 @@ export default function (pi: ExtensionAPI) {
           "agentic-stack pi memory hook: python3/python not found; automatic episodic logging disabled.",
           "warning",
         );
-      } else if (result === "hook-failure" && !warnedHookFailure) {
+      } else if (result === "timeout" && !warnedHookTimeout) {
+        warnedHookTimeout = true;
+        ctx.ui.notify(
+          `agentic-stack pi memory hook timed out (>${HOOK_TIMEOUT_MS}ms); subsequent calls may be skipped. Override with $AGENT_HOOK_TIMEOUT_MS.`,
+          "warning",
+        );
+      } else if (
+        typeof result === "object" &&
+        result.kind === "hook-failure" &&
+        !warnedHookFailure
+      ) {
         warnedHookFailure = true;
+        // Surface the first line of stderr so the failure is diagnosable.
+        const firstLine = result.stderr.split(/\r?\n/, 1)[0] || `(exit ${result.exitCode})`;
         ctx.ui.notify(
-          "agentic-stack pi memory hook failed; continuing without automatic episodic logging.",
+          `agentic-stack pi memory hook failed: ${firstLine}`,
           "warning",
         );
       }
     } catch {
       if (!warnedHookFailure) {
         warnedHookFailure = true;
         ctx.ui.notify(
-          "agentic-stack pi memory hook failed; continuing without automatic episodic logging.",
+          "agentic-stack pi memory hook errored unexpectedly; continuing without automatic episodic logging.",
           "warning",
         );
       }

install.ps1

@@ -154,8 +154,21 @@ switch ($Adapter) {
 
         $skillsSrc = Join-Path $TargetAgent 'skills'
         $skillsDst = Join-Path $piDir 'skills'
-        if (Test-Path $skillsDst) {
-            Remove-Item -LiteralPath $skillsDst -Recurse -Force
+
+        # CRITICAL: detect symlink BEFORE Remove-Item. On PowerShell 5.1
+        # (Windows default), `Remove-Item -Recurse -Force` on a symlink
+        # traverses INTO the target and deletes its contents. Re-running
+        # the installer would silently wipe .agent/skills via the link.
+        # Use IsLink detection + .NET Delete (link only, not target).
+        $skillsDstItem = Get-Item -LiteralPath $skillsDst -Force -ErrorAction SilentlyContinue
+        if ($skillsDstItem) {
+            $isLink = ($skillsDstItem.Attributes -band [System.IO.FileAttributes]::ReparsePoint) -eq [System.IO.FileAttributes]::ReparsePoint
+            if ($isLink) {
+                try { [System.IO.Directory]::Delete($skillsDst, $false) }
+                catch { [System.IO.File]::Delete($skillsDst) }
+            } else {
+                Remove-Item -LiteralPath $skillsDst -Recurse -Force
+            }
         }
         try {
             New-Item -ItemType SymbolicLink -Path $skillsDst -Target $skillsSrc -ErrorAction Stop | Out-Null