HOMELAB-1881: fix(nats-intercom): return Text components from TUI renderers #64

Merged

aaron merged 1 commit from plane/HOMELAB-1881-intercom-renderers into main 2026-10-03 05:18:18 +00:00

Conversation 1 Commits 1 Files changed 3 +49 -29

aaron commented 2026-10-03 05:11:24 +00:00

Owner

Copy link

What

nats-intercom's renderCall/renderResult returned raw strings. pi's ToolDefinition renderer contract (since 0.63.0; enforced fatally in 0.87.1) requires returning a Component — 0.87.1 wraps renderer output in a MouseRegion whose render() calls child.render(width) unguarded.

Impact

Any interactive session that executed the intercom tool crashed the entire pi process with:

TypeError: this.child.render is not a function
    at MouseRegion.render (.../chunk-OJP47DM6.js:384:6850)
    at ToolExecutionComponent.render

Three crashes recorded in ~/.pi/agent/crashes.json (2026-10-02 04:44, 2026-10-03 04:47 + 04:49) — including a session that resumed and re-crashed on the same intercom {action:"list"} batch. HOMELAB-1881.

Fix

Wrap returns in new Text(...) from @earendil-works/pi-tui — the same pattern the todo and pi-swarm-suite renderers use. @earendil-works/pi-tui@0.87.1 added as devDependency (matches pi's bundled copy) so tsc --noEmit resolves types.

Verification

• tsc --noEmit clean
• Post-merge: intercom tool renders in interactive TUI without crashing pi; no new entries in crashes.json

## What `nats-intercom`'s `renderCall`/`renderResult` returned **raw strings**. pi's `ToolDefinition` renderer contract (since 0.63.0; enforced fatally in 0.87.1) requires returning a **Component** — 0.87.1 wraps renderer output in a `MouseRegion` whose `render()` calls `child.render(width)` unguarded. ## Impact Any interactive session that executed the `intercom` tool crashed the entire pi process with: ``` TypeError: this.child.render is not a function at MouseRegion.render (.../chunk-OJP47DM6.js:384:6850) at ToolExecutionComponent.render ``` Three crashes recorded in `~/.pi/agent/crashes.json` (2026-10-02 04:44, 2026-10-03 04:47 + 04:49) — including a session that resumed and re-crashed on the same `intercom {action:"list"}` batch. HOMELAB-1881. ## Fix Wrap returns in `new Text(...)` from `@earendil-works/pi-tui` — the same pattern the `todo` and `pi-swarm-suite` renderers use. `@earendil-works/pi-tui@0.87.1` added as devDependency (matches pi's bundled copy) so `tsc --noEmit` resolves types. ## Verification - [x] `tsc --noEmit` clean - [ ] Post-merge: `intercom` tool renders in interactive TUI without crashing pi; no new entries in `crashes.json`

aaron added 1 commit 2026-10-03 05:11:24 +00:00

HOMELAB-1881: fix(nats-intercom): return Text components from TUI renderers ... dfb147980f

renderCall/renderResult returned raw strings. pi >= 0.87.1 wraps renderer
output in a MouseRegion whose render() calls child.render(width) unguarded,
so any intercom tool call in interactive mode crashed the whole process with
"this.child.render is not a function" (3 crashes in crashes.json, Oct 2-3).
pi's ToolDefinition contract has required Component returns since 0.63.0;
wrap in pi-tui Text like the todo/pi-swarm-suite renderers do.

aaron commented 2026-10-03 05:18:06 +00:00

Author

Owner

Copy link

Landing review — verdict: CLEAN

Reviewed in a fresh context against primary sources only: the PR diff at head dfb147980f57f86822d2bfa57a27b8c16ae0a2fe, pi 0.87.1's installed renderer contract, ~/.pi/agent/crashes.json, and the sibling renderers in todo/ and pi-swarm-suite/. No blockers. 2 non-blocking concerns below.

1. Correctness vs the crash signature — verified, not just read

Every renderer return path is now a Component; there are exactly three and all three were exercised:

Path	Line (head)	Coverage
renderCall (single return)	340	new Text(text, 0, 0)
renderResult isPartial early return	344	new Text(...)
renderResult final return	350	new Text(...)

No unwrapped path remains (no return text; in either hook), and repo-wide renderCall(/renderResult( exists in only three files — this one plus pi-swarm-suite/src/core/index.ts and todo/todo.ts, which already return new Text(...). No other extension carries the same latent defect.

I reproduced the contract both ways in a throwaway probe (fake pi capturing registerTool, wrapping output exactly as pi does):

• Negative control on unfixed main: all 5 probe cases return raw strings, and new MouseRegion("a raw string").render(80) throws TypeError: this.child.render is not a function — byte-identical to the message in all three crashes.json entries (pi 0.87.1, MouseRegion.render → ToolExecutionComponent.render). The crash attribution is confirmed, not assumed.
• Head: 5/5 cases return Text instances and render cleanly through MouseRegion.render(80), including the isPartial early-return path that a naive fix would miss.

2. Crash evidence claim — accurate

All three crashes.json entries are this.child.render is not a function on version 0.87.1, timestamped 2026-10-02T04:44:57Z and 2026-10-03T04:47:57Z / 04:49:37Z — matching the PR description's three crashes. The 04:49 session's last recorded line is an assistant message at 04:49:37.917Z, i.e. 47 ms before the 04:49:37.964Z crash, consistent with the "resumed and re-crashed" claim.

3. Typecheck / tests — reproduced locally

npm ci + npm run typecheck (tsc --noEmit) clean and npm test 48/48 passing at the head sha in a scratch clone, so the added devDependency genuinely resolves Text's types. The claim "@earendil-works/pi-tui@0.87.1 matches pi's bundled copy" checks out: pi-coding-agent 0.87.1 declares "@earendil-works/pi-tui": "^0.87.1" and carries it nested in its own node_modules; the new lock entry resolves 0.87.1.

4. Claim honesty / scope — accurate

Three files changed, all in-scope (index.ts, package.json, package-lock.json). The lockfile additionally drops stale nats/nkeys.js/tweetnacl entries; that is npm reconciling a lockfile that was already inconsistent with package.json (no nats dependency there since the @nats-io/* migration), so it is correct hygiene rather than scope creep. npm ci (which hard-fails on lock/package mismatch) succeeds both locally and in CI, proving the lock is coherent.

---

Concerns (non-blocking)

1. Dependency declaration placement diverges from repo precedent (low). Both sibling extensions that consume Text at runtime declare @earendil-works/pi-tui in dependencies — todo/package.json (^0.80.2) and pi-swarm-suite/package.json (^0.80.2) — whereas this PR places it in devDependencies. Runtime is unaffected either way: pi's extension loader resolves @earendil-works/pi-tui through its virtual-module map to the bundled copy (dist/core/extensions/virtual-modules.js), so the package is only needed for tsc. pi's own docs/packages.md recommends peerDependencies: "*" for supplied Pi packages. Worth aligning in a follow-up so a future npm ci --omit=dev or virtual-map change cannot silently break the import.
2. No in-repo regression test for the contract (low). The renderer hooks remain untested, so the same crash class can return unnoticed elsewhere. My probe shows a cheap guard is possible: load the extension with a fake pi, then assert each hook's result is instanceof Text (and optionally survives a MouseRegion wrap). Suggested follow-up, not a merge gate for a 3-line fix.

CI

Run #124 on head dfb147980f — pass, all 13 jobs green (nats-intercom typecheck + vitest, plus pr-title format gate).

Landing (squash merge + rollout + smoke test) proceeding on this verdict.

## Landing review — verdict: CLEAN Reviewed in a fresh context against primary sources only: the PR diff at head `dfb147980f57f86822d2bfa57a27b8c16ae0a2fe`, pi 0.87.1's installed renderer contract, `~/.pi/agent/crashes.json`, and the sibling renderers in `todo/` and `pi-swarm-suite/`. No blockers. 2 non-blocking concerns below. ### 1. Correctness vs the crash signature — verified, not just read Every renderer return path is now a `Component`; there are exactly three and all three were exercised: | Path | Line (head) | Coverage | |---|---|---| | `renderCall` (single return) | 340 | `new Text(text, 0, 0)` | | `renderResult` `isPartial` early return | 344 | `new Text(...)` | | `renderResult` final return | 350 | `new Text(...)` | No unwrapped path remains (no `return text;` in either hook), and repo-wide `renderCall(`/`renderResult(` exists in only three files — this one plus `pi-swarm-suite/src/core/index.ts` and `todo/todo.ts`, which already return `new Text(...)`. No other extension carries the same latent defect. I reproduced the contract both ways in a throwaway probe (fake `pi` capturing `registerTool`, wrapping output exactly as pi does): - **Negative control on unfixed `main`:** all 5 probe cases return raw strings, and `new MouseRegion("a raw string").render(80)` throws `TypeError: this.child.render is not a function` — byte-identical to the message in all three `crashes.json` entries (pi 0.87.1, `MouseRegion.render` → `ToolExecutionComponent.render`). The crash attribution is confirmed, not assumed. - **Head:** 5/5 cases return `Text` instances and render cleanly through `MouseRegion.render(80)`, including the `isPartial` early-return path that a naive fix would miss. ### 2. Crash evidence claim — accurate All three `crashes.json` entries are `this.child.render is not a function` on version `0.87.1`, timestamped 2026-10-02T04:44:57Z and 2026-10-03T04:47:57Z / 04:49:37Z — matching the PR description's three crashes. The 04:49 session's last recorded line is an assistant message at 04:49:37.917Z, i.e. 47 ms before the 04:49:37.964Z crash, consistent with the "resumed and re-crashed" claim. ### 3. Typecheck / tests — reproduced locally `npm ci` + `npm run typecheck` (`tsc --noEmit`) clean and `npm test` 48/48 passing at the head sha in a scratch clone, so the added devDependency genuinely resolves `Text`'s types. The claim "@earendil-works/pi-tui@0.87.1 matches pi's bundled copy" checks out: `pi-coding-agent` 0.87.1 declares `"@earendil-works/pi-tui": "^0.87.1"` and carries it nested in its own `node_modules`; the new lock entry resolves `0.87.1`. ### 4. Claim honesty / scope — accurate Three files changed, all in-scope (`index.ts`, `package.json`, `package-lock.json`). The lockfile additionally drops stale `nats`/`nkeys.js`/`tweetnacl` entries; that is npm reconciling a lockfile that was already inconsistent with `package.json` (no `nats` dependency there since the `@nats-io/*` migration), so it is correct hygiene rather than scope creep. `npm ci` (which hard-fails on lock/package mismatch) succeeds both locally and in CI, proving the lock is coherent. --- ### Concerns (non-blocking) 1. **Dependency declaration placement diverges from repo precedent (low).** Both sibling extensions that consume `Text` at runtime declare `@earendil-works/pi-tui` in **`dependencies`** — `todo/package.json` (`^0.80.2`) and `pi-swarm-suite/package.json` (`^0.80.2`) — whereas this PR places it in `devDependencies`. Runtime is unaffected either way: pi's extension loader resolves `@earendil-works/pi-tui` through its virtual-module map to the bundled copy (`dist/core/extensions/virtual-modules.js`), so the package is only needed for `tsc`. `pi`'s own `docs/packages.md` recommends `peerDependencies: "*"` for supplied Pi packages. Worth aligning in a follow-up so a future `npm ci --omit=dev` or virtual-map change cannot silently break the import. 2. **No in-repo regression test for the contract (low).** The renderer hooks remain untested, so the same crash class can return unnoticed elsewhere. My probe shows a cheap guard is possible: load the extension with a fake `pi`, then assert each hook's result is `instanceof Text` (and optionally survives a `MouseRegion` wrap). Suggested follow-up, not a merge gate for a 3-line fix. ### CI Run **#124** on head `dfb147980f` — **pass**, all 13 jobs green (`nats-intercom` typecheck + vitest, plus `pr-title` format gate). Landing (squash merge + rollout + smoke test) proceeding on this verdict.

aaron merged commit 567dfa8202 into main 2026-10-03 05:18:18 +00:00

aaron referenced this pull request from a commit 2026-10-03 05:18:18 +00:00

HOMELAB-1881: fix(nats-intercom): return Text components from TUI renderers (#64)

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels

No items

No labels

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

Homelab/pi-extensions!64

No description provided.