Repository navigation
Render AgentTab tool calls as native Tern cards in OMP - #59
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![wolf-maintainer[bot]](https://avatars.githubusercontent.com/in/4804189?s=60&v=4){kind=small}
There was a problem hiding this comment.
P2 — coherent native-card feature and the node shapes match the OMP/TSP contracts, but this changes the default OMP presentation and needs the maintainer call.
Should fix before merge: native head/detail strings bypass existing TUI normalization and per-line bounds; the OMP-registration/Pi-isolation contract lacks coverage.
Targeted native/render tests pass; the full package suite is blocked locally because typebox is absent.
Thanks for the focused implementation.
Not anchored to diff
packages/omp/src/index.ts:487— should-fix: No test reaches this registration branch.native.test.tsinvokes the helpers directly, so removingdescribeCall,describeResult, ormergeCallAndResult—or accidentally exposing them in Pi mode—would still pass while breaking the feature and the stated "Pi adapter unchanged" contract. Extend the existing registration tests to assert all three fields in OMP mode and their absence in Pi mode.
| const meta = [ | ||
| ...(outcome === undefined ? [] : [outcome]), | ||
| ...contextMeta({ ...card.context, taskId: undefined, owned: false }), | ||
| ]; | ||
| const badge = ATTENTION_BADGE[card.status]; | ||
| const target = call.meta.join(" · "); | ||
| return { | ||
| title: call.title, | ||
| ...(target ? { target, targetKind: "text" as const } : {}), |
There was a problem hiding this comment.
should-fix: These head strings bypass the terminal-safe normalization that renderHeader() applies. For example, describeCallView("browser_wait", { condition: { kind: "text", value: "A\tB" } }) emits the tab verbatim in tool.target, and an error containing a tab is copied verbatim into tool.meta. That violates the TUI contract that every render path replace tabs/control characters and bound displayed text. Normalize and truncate the native head fields before returning them.
| return { | ||
| k: "section", | ||
| p: { head: [{ t: "Result", s: "muted" }, { t: " redacted", s: "dim" }], collapsible: true, collapsed: !expanded }, | ||
| c: [{ k: "code", p: { text: expandedLines(payload).join("\n"), lang: "json" } }], |
There was a problem hiding this comment.
should-fix: expandedLines() caps only the number of lines; the ANSI renderer then truncates every line to the viewport, but this native path joins each full line directly into the TSP code node. A single long result value therefore remains a single multi-kilobyte line (and control characters other than redacted keys remain intact), contrary to the claimed bounded-detail/TUI sanitization contract. Bound each native detail line before serializing it.
1 participant
Summary
describeCall,describeResult,mergeCallAndResult) so Tern renders each browser action as one merged card.packages/omp/src/native.tswith per-tool titles, status, and bounded detail lines; reuse the existing redaction and context metadata fromrender.ts.Verification
bun run typecheck,bun test, andbun run buildinpackages/omp.browser_tabsend to end.