Skip to content

render crew's own notices under crew's icon and label - #171

Merged
nikolasd merged 6 commits into
mainfrom
crew-131-crew-branded-notices
Sep 11, 2026
Merged

nikolasd merged 6 commits into
mainfrom
crew-131-crew-branded-notices

Conversation

@nikolasd

Copy link
Copy Markdown
Owner

Every digest the milestone bridge injects went through pi.sendMessage as a bare string. A bare string is oh-my-pi's shorthand for a CustomMessagePayload that omits customType, which the runtime normalizes to DEFAULT_CUSTOM_MESSAGE_TYPE ("custom-message") — the same generic type every other extension's un-typed message falls back to, rendered under a header naming no extension at all. A crew notice was indistinguishable from any other extension's.

What changed

  • The bridge now sends { customType: "crew", content } on any omp build that proves it supports custom renderers (see below). Digest wording itself is unchanged.
  • attachMilestoneBridge registers a renderer for that type via pi.registerMessageRenderer — a small box with crew's own icon and label as its header, the digest text as its body. Label text ("Crew") is the maintainer's agreed default, pending final confirmation on the exact wording; the icon is a placeholder pick for the same reason.

One capability check gates both the wire shape and the module

Two review findings shared one root: the code was carefully defensive about the method (registerMessageRenderer accessed through a cast, degrading to "no custom renderer" when absent) but not defensive about the two things that method's presence was meant to stand in for.

Finding 1 — the wire shape. The { customType, content } object was sent unconditionally. The package's peer range is >=17.0.7 <19, and nobody has established that a build as old as 17.0.7 accepts anything but a bare string — if it doesn't, the digest isn't "less legible" as the code claimed, it's garbled or dropped. Fixed: supportsCustomMessageRenderer(pi) is computed once at attach and read at both the registration call site and the send call site. A build without the method gets a bare-string digest, unconditionally.

Finding 2 — the module. Box/Text were a static top-level import of @oh-my-pi/pi-tui, a module the build marks --external. A static import of an external specifier is evaluated at load time, unconditionally — so a build that doesn't expose that module to extensions would fail the whole extension's load, every tool and the monitor included, not just this renderer's header. Fixed: the import moved to a dynamic await import(...) inside registerCrewMessageRenderer, itself only ever called after the same capability check. A resolution failure there (or the host's own registerMessageRenderer rejecting the renderer, covered by the same try/catch) degrades exactly like the method being absent.

Together these make the real promise provable by reading the code: on any build without custom-renderer support, the digest still arrives as plain text, unconditionally, and nothing else about the extension is at risk.

@oh-my-pi/pi-tui is a direct peer/dev dependency and a second --external in the build — registerMessageRenderer's own return type (Component) is pi-tui's, so any extension implementing this method already needs pi-tui's types regardless of how the import is structured.

Live-render gap, stated plainly

Live rendering in a real interactive omp session was not exercised by the author — this sandbox's nested PTY produces zero output for any interactive omp launch (omp --version responds instantly, so it isn't the binary). What's verified instead: the actual registerMessageRenderer call path through real @oh-my-pi/pi-tui Box/Text components (not mocks), with the rendered output asserted against the real string and confirmed to discriminate (swapped the icon, watched the assertion fail on the real render). The live proof is deferred to the protocol spike's E2E now that the terminal-path attempt is cancelled in favor of the protocol-first pivot.

Tests

Registration itself; the digest's wire shape (object when supported, bare string when not — each verified to discriminate); the renderer's real output on a plain-string digest and on a rich-content payload it must still degrade gracefully on; the combined try/catch covering both the dynamic import and the register.call that follows it; a build with no registerMessageRenderer attaching without throwing.

Gates

bun run typecheck, bun run format:check, the extension build (confirmed via the built bundle: zero static from "@oh-my-pi/pi-tui" imports remain), bun test, and cargo test --workspace all green on the committed tip, merged with origin/main at 02d90c0.

nikolasd and others added 6 commits September 11, 2026 00:23
Every digest the milestone bridge injects went through `pi.sendMessage`
as a bare string. A bare string is oh-my-pi's shorthand for a
`CustomMessagePayload` that omits `customType`, which the runtime
normalizes to `DEFAULT_CUSTOM_MESSAGE_TYPE` ("custom-message") -- the
same generic type every other extension's un-typed message falls back
to, rendered under a header naming no extension at all. A crew notice
was indistinguishable from any other extension's.

Two changes close that:

- The bridge now sends `{ customType: "crew", content }` instead of a
  bare string. Digest wording itself is unchanged.
- `attachMilestoneBridge` registers a renderer for that type via
  `pi.registerMessageRenderer` -- a small box with crew's own icon and
  label as its header, the digest text as its body. Label text ("Crew")
  is the maintainer's agreed default, pending final confirmation on the
  exact wording; the icon is a placeholder pick for the same reason.

Both lookups follow the same defensive-cast convention already
established here for `sendMessage`: an older or renamed omp method
degrades to "no custom renderer" (the generic fallback header) rather
than a crash. Unlike a missing `sendMessage`, a missing
`registerMessageRenderer` never warns -- losing it is cosmetic (the
digest still arrives, under a less legible header), not a silently
dropped notification.

`@oh-my-pi/pi-tui` becomes a direct peer/dev dependency and a second
`--external` in the build: `registerMessageRenderer`'s own return type
(`Component`) is pi-tui's, so any extension implementing this method
already needs pi-tui's types regardless -- this isn't reaching past the
documented extension surface, it's the shape that surface requires.
pi-coding-agent depends on pi-tui itself the same way (a hard
dependency at the same version), so it should resolve identically to
how pi-coding-agent already does for this extension, at runtime.

Verification gap, stated plainly rather than glossed: live rendering in
an interactive omp session was not exercised by the author. This
sandbox's nested PTY produces no interactive terminal output at all --
confirmed with a bare `omp --allow-home` and no extension involved,
`omp --version` responding instantly the whole time, so the omp binary
itself is fine and the gap is this environment's PTY handling, not the
code. What IS verified: the actual `registerMessageRenderer` call path
this bridge uses, exercised through real `@oh-my-pi/pi-tui` `Box`/`Text`
components (not mocks), with the renderer's real rendered output
asserted to contain the icon, the label, and the digest text verbatim --
and confirmed to discriminate: swapping the icon constant and re-running
the test fails on the actual rendered string, not a stub. The live proof
is a maintainer-run check in the first real session after this merges.

Tests: registration itself (customType "crew", called once at attach);
the digest's own wire shape now carries that customType, not a bare
string; the registered renderer's real output on both a plain-string
digest and a rich-content payload it must still degrade gracefully on;
an omp build lacking `registerMessageRenderer` attaches without
throwing.

cargo fmt --all, cargo clippy --all-targets --all-features -D warnings,
bun run check (markers, generate --check, format:check, typecheck,
extension build with pi-tui confirmed external not bundled, bun test,
cargo test --workspace all green) all verified clean.
…ility check

Two findings from review, sharing one root: the code was carefully
defensive about the *method* (`registerMessageRenderer` accessed through
a cast, degrading to "no custom renderer" when absent) but not defensive
about the two things that method's presence was supposed to stand in
for -- the wire shape `sendMessage` gets, and the module the renderer
itself needs. The degradation the code's own comment promised did not
actually hold end to end.

Finding 1: the `{ customType, content }` object was sent to `sendMessage`
unconditionally, regardless of whether `registerMessageRenderer` existed.
The package's own peer range is `>=17.0.7 <19`, and nobody has established
that a build as old as 17.0.7 accepts anything but a bare string there --
if it does not, the digest is not "less legible" as promised, it is
garbled or dropped, which is exactly the silent-notification-loss this
file works hard to prevent elsewhere. Fixed by gating the wire shape on
the same capability check the renderer registration already uses: a
build without `registerMessageRenderer` is exactly the build most likely
to also predate `customType` support, so its presence is a sound signal
for both rather than two things to verify independently. That check is
now computed once, in `supportsCustomMessageRenderer`, and read at both
call sites.

Finding 2: `Box`/`Text` were a static top-level import of
`@oh-my-pi/pi-tui`, a module the build marks `--external`. A static
import of an external specifier is evaluated at load time, unconditionally
-- so a build that does not expose that module to extensions would fail
the whole extension's load, every tool and the monitor included, not
just this one renderer's header, and would pay that cost even on a build
that never calls the renderer at all. That is a strictly worse failure
mode than anything this file was trying to prevent. Fixed by moving the
import behind a dynamic `await import(...)` inside
`registerCrewMessageRenderer`, itself only ever called after the same
capability check above -- so the module is only required on a build that
has already proven it supports custom renderers, and a resolution
failure there (or the host's own `registerMessageRenderer` rejecting the
renderer, guarded by the same try/catch) degrades exactly like the
method being absent: no custom renderer, the fallback header applies,
nothing else breaks.

Together these make the file's real promise provable by reading the
code rather than by knowing which omp versions accept what: on any build
without custom-renderer support, the digest still arrives as plain text,
unconditionally, and nothing else about the extension is at risk.

Tests: a new case pins the wire-shape gate directly -- a build with no
`registerMessageRenderer` must receive a bare-string digest, never the
object shape, verified to discriminate (temporarily made the shape
unconditional, watched the assertion fail on the real sent value: object
instead of string). The three existing renderer tests now await an
async flush, since registration happens after a dynamic import resolves
rather than synchronously at attach time. A new case exercises the
combined try/catch around both the import and the `register.call` that
follows it, confirming either failing degrades quietly rather than
throwing at attach time.

bun run typecheck, bun run format:check, the extension build (confirmed
via the built bundle: zero static `from "@oh-my-pi/pi-tui"` imports
remain, only the dynamic `await import(...)` inside the gated
function), and bun test (all green) verified clean.
…s safe

The wire-shape decision `sendMessage` uses is computed synchronously at
attach time, before the async renderer registration is even started --
so a digest arriving in the brief window before that registration's
dynamic import resolves still gets the correct `{ customType, content }`
object, just under the generic fallback header until the import settles.
No correctness consequence, bounded to attach-time milliseconds well
before any run reaches a milestone. Worth saying at the call site rather
than leaving a future reader to re-derive it from the two functions'
relative timing.

No behavior change. bun run typecheck and the milestones test suite
(29/29) verified clean.
@nikolasd
nikolasd merged commit 8456bf3 into main Sep 11, 2026
19 checks passed
@nikolasd
nikolasd deleted the crew-131-crew-branded-notices branch September 11, 2026 11:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant