Add pi tool-result hook and windows installer - #17
Conversation
| import { fileURLToPath } from "node:url"; | ||
|
|
||
| const EXTENSION_DIR = path.dirname(fileURLToPath(import.meta.url)); | ||
| const PROJECT_ROOT = path.resolve(EXTENSION_DIR, "..", ".."); |
There was a problem hiding this comment.
import.meta.url may not resolve correctly under jiti. Pi loads extensions via jiti, which in CJS-transformed mode can leave import.meta.url as undefined — silently breaking EXTENSION_DIR and disabling the hook entirely on install for some users. __dirname is the safe choice here:
const EXTENSION_DIR = path.dirname(__filename);|
|
||
| export default function (pi: ExtensionAPI) { | ||
| pi.on("tool_result", async (event, ctx) => { | ||
| const payload = { |
There was a problem hiding this comment.
No pre-filtering before runHook() means a Python subprocess is spawned for every read call, only to be discarded inside pi_post_tool.py. Worth adding a guard here to match the Claude Code hook's matcher behaviour and avoid the subprocess overhead on noise:
if (!['bash', 'edit', 'write'].includes(event.toolName)) return;| from hooks.post_execution import log_execution # noqa: E402 | ||
| from hooks.on_failure import on_failure # noqa: E402 | ||
| import hooks.claude_code_post_tool as cc # noqa: E402 | ||
|
|
There was a problem hiding this comment.
This imports private underscore-prefixed functions from claude_code_post_tool.py. If those internals get refactored the pi hook breaks silently with no type error or warning. Worth either making _is_success, _importance, _action_label, _reflection, _detail, and _pain_score public, or extracting them into a shared _scoring.py module both hooks import from.
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 codejunkie99#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 codejunkie99#3) and the rsync-style sync for .pi/skills (Codex codejunkie99#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.
|
Hey @hovhannest — ran a cross-model review (Claude + Codex adversarial) on this and found one data-loss bug and three silent-degradation paths. Pushed a follow-up commit ( What changed:
Tested: empty payload, malformed JSON, Pi camelCase Edit, well-formed bash success, idempotent re-run. |
Final codex review surfaced two P2s that would break upgrades and downstream salience. 1. install.sh:148 / install.ps1 — the top-level `.agent/` copy is skipped when the target already has one (line 39-42 / 45-48). So on an upgrade install of an older agentic-stack project, the pi extension's `memory-hook.ts` gets dropped, but the Python hook it invokes (`.agent/harness/hooks/pi_post_tool.py`) never arrives. Every `tool_result` then fires `missing-hook` forever. Fix: sync `pi_post_tool.py` explicitly in the pi case, both installers. 2. pi_post_tool.py:152 — `_emit_malformed()` was writing `importance="medium"` (string). Downstream `salience_score()` does `importance / 10.0`, which raises TypeError on a string and would crash context_budget.py, show.py, and auto_dream.py readers of AGENT_LEARNINGS.jsonl. Fix: use int 5 (middle of the 1-10 scale). Smoke-tested: - empty payload → entry has `importance: 5` (int), salience_score runs to 1.000 without error - upgrade install on a project with old .agent/ (no pi_post_tool.py) → "synced for upgrades" line fires, file now present
Two pre-existing infrastructure bugs flagged during the PR codejunkie99#17 cross-model review, fixed here against master because they predate that PR and affect every harness. ## 1. Concurrent writes to AGENT_LEARNINGS.jsonl post_execution.py and on_failure.py both did plain `open(EPISODIC, "a")` → `f.write(json.dumps(entry) + "\n")`. POSIX O_APPEND makes single `write(2)` calls atomic only up to PIPE_BUF (4 KB on Linux/macOS). In practice most entries stay under that ceiling and the unlocked code never corrupts, but the `reflection` field is uncapped in log_execution and can easily exceed 4 KB on high-importance failure logs. Every downstream reader (auto_dream.py, cluster.py, context_budget.py, show.py) skips `json.JSONDecodeError` lines silently — so one over-PIPE_BUF interleave = one episodic entry gone with no signal. Fix: new `_episodic_io.append_jsonl()` helper that opens in append- binary mode (no Python text-mode buffering quirks) and wraps the write in `fcntl.flock(LOCK_EX)`. Shared by both writers. On platforms without fcntl (native Windows Python) behavior falls back to the pre-fix unlocked append; WSL, git-bash/Cygwin, macOS, Linux all have fcntl. Verified: 40 concurrent writers × 500 entries × 2 KB reflection each → 20,000 parseable lines, zero corruption. ## 2. pi install.sh silently leaves stale skills on re-install `ln -sfn src dest` where `dest` is a REAL directory (e.g. from an earlier copy-fallback install) silently creates `dest/<basename-of-src>` INSIDE the dir and exits 0. The existing `if ln -sfn; then` branch took the success path, the `rm -rf + cp` fallback never ran, and orphans stuck around forever. Verified on macOS, confirmed the symlink-inside-dir behavior. Fix: check `-L` (symlink) and `-d` (real dir) explicitly before calling `ln -sfn`, mirror the pattern used by the codex adapter (PR codejunkie99#16 follow-up). Existing symlink → cheap repoint. Real directory → rsync --delete when available, rm+cp otherwise. Non-existent → symlink or copy fallback. Same three-branch shape, no more silent wrong behavior. Verified: re-install after orphan-skill was added to a real-dir `.pi/skills` → rsync --delete removes the orphan.
#24) * fix(pi): rewrite adapter — inline TS hook, formula crash, decay tz bug Fixes four distinct issues found after PR #17 was merged: ## 1. ModuleNotFoundError crash on brew install (Formula) Formula/agentic-stack.rb pkgshare.install was missing harness_manager/. install.sh dispatches to `python3 -m harness_manager.cli` but the module was never copied into the cellar, causing an immediate crash for every brew user regardless of adapter. ## 2. memory-hook.ts — complete rewrite (approach was wrong) PR #17's hook used: - fileURLToPath(import.meta.url) → jiti leaves this undefined in CJS-transform mode, silently breaking HOOK_SCRIPT path resolution - spawn(python3, [pi_post_tool.py]) per tool_result → subprocess overhead + timeout complexity for every single tool call - No pre-filtering → Python was spawned for read/find/ls/grep too - Private cc._is_success / cc._importance etc. imports → breaks silently on any refactor of claude_code_post_tool.py New approach (matches the working life-international reference setup): - process.cwd() for all path resolution — no import.meta.url - All importance / pain_score / action / reflection logic is inline TypeScript — no Python subprocess per tool call - Direct fs.appendFileSync to AGENT_LEARNINGS.jsonl - Pre-filter: bash / edit / write only (mirrors Claude Code's "^(Bash|Edit|Write)$" matcher). Low-importance bash successes (imp <= 3) are also skipped to keep the log signal-rich. - Typed inputs via isBashToolResult / isEditToolResult / isWriteToolResult type guards (exported from pi's public API) - cachedSha typed as string | undefined so the git cache actually works - session_shutdown handler runs auto_dream.py on quit/new/resume, mirroring Claude Code's Stop hook. cron is now an optional fallback. ## 3. adapter.json — remove from_stack pi_post_tool.py entry The hook is self-contained TypeScript. pi_post_tool.py is no longer deployed as an explicitly-managed adapter file. It remains in the brain template for standalone/debug use. ## 4. decay.py — timezone-aware vs naive datetime crash auto_dream.py → decay_old_entries() compared a timezone-aware cutoff (datetime.now()) against naive timestamps from AGENT_LEARNINGS.jsonl, crashing with TypeError. Fixed by using datetime.now(timezone.utc) and normalising naive timestamps to UTC before comparison. Tested: fresh install on lumen-review, 41/41 tests pass, doctor green. * fix(pi): wire shutdown hook, fix edit reflection, normalise tz, atomic dream cycle Follow-up fixes for PR #24 surfaced by independent verification (memory-hook type cross-reference + codex review). Each fix is annotated with the failure mode it addresses. P0 — session_shutdown filter rejected every event Pi's `SessionShutdownEvent` is `{ type: "session_shutdown" }` with no `reason` field (verified in pi-coding-agent's types.d.ts and the emit site at agent-session.js:1638). The hook's `DREAM_REASONS.has(event.reason)` filter therefore evaluated `has(undefined) === false` for every event, so `auto_dream.py` never ran. Drop the filter; shutdown only fires once on process exit. Add a re-entrancy guard for defence in depth. P1 — edit reflection always lost the diff Hook accessed `event.input.edits[0]` but Pi's `EditToolInput` is flat `{ path, oldText, newText }` (no `edits` array — that's MultiEdit on Claude Code). Reflections silently degraded to `Edited <path>` with no old/new content for the dream-cycle clusterer to grip. Use the flat fields directly. P1 — naive-local Python timestamps + UTC decay = silent drift Decay's "naive == UTC" assumption is correct only if writers emit UTC. They didn't: post_execution, on_failure, learn, graduate, promote, review_state, render_lessons all wrote naive-local. Switch every writer to `datetime.now(timezone.utc).isoformat()` and teach every reader (salience, show._human_age / _daily_counts / failing_skills / last_dream_cycle, on_failure._count_recent_failures, review_state._age_factor, archive) to normalise naive timestamps to UTC before comparing. P1 — one bad user regex disabled every user pattern Pre-fix: build one combined RegExp per list, catch any error, return null for both. A single typo in `hook_patterns.json` silently dropped all user rules. Now: validate per-fragment, incremental merge — same posture as `claude_code_post_tool.py`'s `_filter_valid` / `_build_with_fallback`. P1 — auto_dream lost entries that landed mid-cycle Original PR fixed the truncate-before-lock race in `_write_entries` but not the read-modify-write window: an `append_jsonl()` between `_load_entries()` and `_write_entries(kept)` would be truncated away. Hold a single LOCK_EX on the episodic log across the entire cycle via `_episodic_locked()`. Mutually exclusive with `_episodic_io.append_jsonl` (same flock target). P1 — salience over-scored future-skewed legacy rows Legacy naive-local timestamps re-interpreted as UTC can read as a few hours in the future during the migration window. `timedelta.days` then went negative and `recency = 10 - age*0.3` exceeded the intended cap. Floor age at 0; clamp recency to ≤ 10. P2 — _cachedSha went stale across `git commit` inside a session Cache for the lifetime of pi was a perf win but recorded the pre-commit SHA on every entry after a mid-session commit. Invalidate on bash commands matching `git <subcmd>` for HEAD-moving subcommands. `[^|;&]*?` allows option flags between `git` and the subcommand (`git -c key=val checkout main`, `git -C path switch dev`). Includes `switch` which an earlier draft missed. P3 — test_pi_install_creates_symlink_and_syncs_hook misnamed Asserted that `pi_post_tool.py` was synced via from_stack — the entry PR #24 removed. Rename the test, drop the obsolete assertion, document in the comment that the .py still ships in the brain template for standalone use. P3 — decay archive filename used local date while cutoff was UTC Asymmetric. `archive_{date}.jsonl` now uses UTC date. Plus tests/test_decay_timezone.py — six tests pinning the shapes decay must handle: aware UTC old/recent, naive old, mixed mid-stream, malformed, archive-filename-uses-UTC. Catches the original crash and the migration-era silent-drift cases. Test suite: 47 / 47 pass (was 41 before; +6 new). Lock semantics verified manually (open-then-flock-then-ftruncate; full read-modify-write window held under a single fd). SHA-invalidation regex spot-checked against 12 git command shapes. --------- Co-authored-by: codejunkie99 <email@email.com>
Summary
Add automatic episodic logging for the Pi adapter and add native Windows PowerShell installer support for
pi.Why
This addresses the gap described in issue #13: Pi already exposes a
tool_resultextension event, but the adapter was not using it, soAGENT_LEARNINGS.jsonlstayed empty unless the model manually remembered to callmemory_reflect.py.What changed
.pi/extensions/memory-hook.tsto the Pi adapter.agent/harness/hooks/pi_post_tool.pyto normalize Pitool_resultevents into the shared episodic logging pathinstall.shto install the Pi extensionpisupport toinstall.ps1tool_resultValidation
install.ps1 pi <target> -Yesin a temp project on Windows.pi/extensions/memory-hook.tsis installedtool_resultpayload throughpi_post_tool.pyskill: "pi"and a derived action labelCloses #13.