feat(connector-oh-my-pi): render cotal_* tool calls natively (no spinner fallback) - #11
Closed
sealedsecurity-bot wants to merge 2 commits into
Closed
Conversation
…ner fallback)
The cotal_* tools registered via `pi.registerTool` carried no `renderCall`, so
OMP fell back to its generic animated-spinner glyph on every tool card. Attach a
`renderCall` to both registration branches (the `cotal_inbox` read-only branch
and the generic branch) in `registerSpec`: returning a Component takes OMP's
custom-renderer branch instead of the spinner fallback.
The renderer is a titled single line — the tool label plus a per-surface summary
drawn from args (recipient for cotal_dm, channel + mentions for cotal_send, role
for cotal_anycast, status/attention/activity for cotal_status, a static label for
cotal_inbox). `cotalCallSummary` is pure and exported for the smoke.
Component is imported type-only from `@oh-my-pi/pi-tui` and the renderer returns a
minimal hand-rolled Component (only `render(width)` is required by the interface)
rather than pi-tui's `Text`: a runtime pi-tui import drags `@oh-my-pi/pi-utils`'
Bun-coupled barrel (`import { YAML } from "bun"`), which the Node/tsx smoke can't
load. pi-tui is therefore a type-only devDependency.
Workstream A of the native-renderer design (docs/designs/platform/cotal-connector-renderer.md);
the inbound message renderer (Workstream B) follows separately. Extends the
extension smoke to assert every registered tool exposes a renderCall (the
spinner-fallback regression guard) and that the summary is arg-enriched.
Co-Authored-By: seal <noreply@sealedsecurity.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe extension adds compact, width-aware renderers and argument-based summaries for all registered ChangesCotal tool-call rendering
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant RegisteredCotalTool
participant cotalCallSummary
participant renderCotalCall
participant OMPUI
RegisteredCotalTool->>cotalCallSummary: provide tool name, args, and default channel
cotalCallSummary-->>renderCotalCall: return call summary
renderCotalCall-->>OMPUI: return Component
OMPUI->>renderCotalCall: request render(width)
renderCotalCall-->>OMPUI: return truncated line
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThis PR adds native Oh My Pi rendering for the
Confidence Score: 5/5This looks safe to merge.
Important Files Changed
Reviews (2): Last reviewed commit: "fix(connector-oh-my-pi): cotal_send card..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
extensions/connector-oh-my-pi/src/extension.ts (1)
145-198: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueLGTM! The defensive arg parsing in
cotalCallSummaryand the minimal hand-rolledComponentinrenderCotalCallare clean and well-documented.One optional nit: the truncation-with-ellipsis pattern (
slice(0, n - 1) + "…") appears in bothpreview(line 154) andrenderCotalCall(line 194). A sharedtruncate(s, max)helper would eliminate the duplication and keep the edge-case handling consistent. Feel free to defer.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@extensions/connector-oh-my-pi/src/extension.ts` around lines 145 - 198, Optionally extract the duplicated truncation-with-ellipsis logic from cotalCallSummary’s preview and renderCotalCall into a shared truncate(s, max) helper, preserving current handling for zero or narrow widths and updating both call sites to use it.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@extensions/connector-oh-my-pi/src/extension.ts`:
- Around line 145-198: Optionally extract the duplicated
truncation-with-ellipsis logic from cotalCallSummary’s preview and
renderCotalCall into a shared truncate(s, max) helper, preserving current
handling for zero or narrow widths and updating both call sites to use it.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 45b079a0-2a69-476e-b8c0-27ba6c3f59c2
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (3)
extensions/connector-oh-my-pi/oh-my-pi-extension.smoke.tsextensions/connector-oh-my-pi/package.jsonextensions/connector-oh-my-pi/src/extension.ts
…mp zero-width render Two display-correctness fixes on the cotal_* tool-call renderer, from PR review: - Send cards named the wrong channel: the renderer hardcoded `#general` for an omitted `channel`, but the tool resolves the omitted case to the agent's configured default (`config.subscribe.find(isConcreteChannel) ?? "general"`, per CotalEndpoint.multicast). An agent whose default was not `general` saw a card claiming `#general` while the message went elsewhere. Thread the same resolved default into `cotalCallSummary` so the card names the true target. - Zero-width render slot bypassed truncation: a `width <= 0` (or non-finite) slot fell through to `line.length` and returned the untruncated title, violating the width-bounded render contract. Clamp to a single empty line. - Extract the shared `truncate(s, max)` helper used by both the summary preview and the render clamp, so the narrow/zero-width edges are handled identically. Extends the extension smoke (3c): omitted send channel shows the resolved default (not a fabricated `#general`), explicit channel still wins, and a zero-width render returns `[""]`. Red-green verified. Co-Authored-By: seal <noreply@sealedsecurity.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Workstream A of the native OMP renderer design (
docs/designs/platform/cotal-connector-renderer.md): give thecotal_*tools arenderCallso their tool cards render natively instead of falling back to OMP's generic animated-spinner glyph (the artifact Matt flagged).renderCallto both registration branches inregisterSpec— thecotal_inboxread-only branch and the generic branch. Returning aComponentis what takes OMP's custom-renderer path instead of the spinner fallback.cotal_dm, channel + mentions forcotal_send, role forcotal_anycast, status/attention/activity forcotal_status, a static label forcotal_inbox.cotalCallSummaryis pure and exported for the smoke.renderCall(the spinner-fallback regression guard), and the summary is arg-enriched. Red-green verified (removing the genericrenderCallfails the new assertion).Scope
extensions/connector-oh-my-pi(+ a type-only devDependency). No core/protocol change.content(the LLM-visible text / non-TUI fallback) is untouched.cotal:incoming/cotal:nudgemessage renderer) is intentionally not here — it ships separately per the design's split-delivery decision.Verification
pnpm --filter @cotal-ai/oh-my-pi build— tsc + esbuild bundle clean.pnpm --filter @cotal-ai/oh-my-pi test— all three connector smokes green (peer, extension, interactive-loop).renderCall-present assertion fails when the hook is removed, passes with it.Open Questions (parked for Matt — overnight)
[LOAD-BEARING — landing target] This PR is stacked on fix(connector): deliver peer messages as nextTurn, not steer #8 (
cotal-connector-esc-composer), notmain. The connector code (registerSpecinextension.ts) exists only on fix(connector): deliver peer messages as nextTurn, not steer #8, which is fork-base held (stacked on feat(oh-my-pi): Cotal connector — headless peer + interactive extension #5). Implemented against the design's own recommendation (OQ#2 option (a): stack + park, execute-ready the moment the fork-base clears) rather than waiting for fix(connector): deliver peer messages as nextTurn, not steer #8 →main. If you'd rather it wait for fix(connector): deliver peer messages as nextTurn, not steer #8 to land and re-base offmain, say so and I'll retarget. Verified:extension.tsis absent onorigin/main, so basing onmaintoday is not possible without fix(connector): deliver peer messages as nextTurn, not steer #8.[implementation refinement, non-load-bearing]
Componentis imported type-only; the renderer returns a minimal hand-rolledComponentrather than pi-tui'sText. The design named@oh-my-pi/pi-tuiprimitives, but a runtime pi-tui import drags@oh-my-pi/pi-utils' barrel, which doesimport { YAML } from "bun"— unloadable under the connector's Node/tsx smoke harness (the real OMP runtime is Bun, soTextwould work there, but CI smokes run on Node). TheComponentinterface requires onlyrender(width): readonly string[], so a hand-rolled one is a valid Component, keeps the connector's existing type-only-OMP discipline, and avoids the Bun coupling.@oh-my-pi/pi-tuiis therefore a type-only devDependency. If you'd prefer realText+ a Bun-run smoke, that's the alternative.Co-Authored-By: seal noreply@sealedsecurity.com