Bot Tools: draft/bot-tools + draft/bot-cmds (IRCv3 spec)#238
Open
ValwareIRC wants to merge 56 commits into
Open
Bot Tools: draft/bot-tools + draft/bot-cmds (IRCv3 spec)#238ValwareIRC wants to merge 56 commits into
ValwareIRC wants to merge 56 commits into
Conversation
Implements client-side support for the draft/ai-tools v0.4 spec: AI bots stream their workflow state (thinking, tool calls, results) as TAGMSG carrying a +obby.world/ai-tools JSON envelope; we render those workflows in a floating tray pinned to the top-right of the chat area, scoped to the current channel. Each workflow is its own card -- spinner + bot nick + dropdown chevron when collapsed, expands to show a Claude-Code-style timeline of steps (colored dot accent per type, content rendered in a monospace box for both string fragments and tool-call args). Multiple bots → multiple stacked cards, so workflow telemetry never eats into chat real-estate. Control signals (cancel / approve / reject) round-trip via TAGMSG to the bot's nick. Pending-approval steps surface inline buttons inside the card; the card header has a Stop button while running and a dismiss-X after the workflow reaches a terminal state. Includes: - aiTools.ts: decoder/encoder + TS types matching the spec - src/store/handlers/aiTools.ts: TAGMSG/CHANMSG/USERMSG → workflow state machine; string-content fragments concatenate so the ai-tools/content-stream batch reassembles correctly even when processed message-by-message - AiToolsCard + AiToolsTray: the floating UI - ChatArea wires the tray in, scoped to the selected target - draft/ai-tools added to the CAP REQ list - Tests: 21 new (decoder edge cases + handler state transitions) - Translations: all 18 supported locales
Replaces the flat JSON.stringify <pre> dump with a key→value chip tree. Primitives become inline colored chips (green strings, cyan numbers, purple bools); nested objects/arrays nest under a tinted left rule with their entries laid out one-per-row. Much easier to scan tool-call args once they get more than a couple of fields, which the user noticed when the bot started passing structured options through.
'Workflow history' (button title) and '{0} step(s)' (list row summary)
across all 18 non-English locales.
CollapsibleMessage renders as a block-level div, so the inline-flex pill stacked above the message body rather than sitting next to it. Float-left makes the body wrap around the chip, prepending it to the first line as intended.
Extends the workflow message schema with an optional `prompt` field that bots can use to ship a short truncated copy of the trigger message. The card renders it in italic muted text under the workflow name so users see what was asked without scrolling back to the trigger PRIVMSG. Field is optional -- bots that don't include it are unaffected, the card just omits the line. Plumbed end-to-end: lib/aiTools.ts decoder, AiWorkflow store type, applyWorkflowUpdate merge, AiToolsCard render.
Going from 60s to 5s means the previous 'fade only in the last 10s' logic would start with the card already half-transparent, so switch the opacity easing to span the full countdown (1.0 -> 0.15) instead.
Live workflows now claim a chat slot the moment the start TAGMSG lands. The slot shows the workflow name + a live preview of the latest step with a spinner, then morphs in place into the bot's final PRIVMSG (carrying the workflow pill) once it arrives -- no row jump. Historical (chathistory-replayed) workflows no longer pop floating tray cards on channel join; they remain in the workflow history popover and the inline pill on the original message.
…holder Two issues with the in-chat placeholder flow: 1. When the bot's tagged PRIVMSG morphs the placeholder, we were not resolving the +reply tag, so the morphed row had no reply quote block (whereas an untagged reply landed via the normal path with quote intact). Now resolveReplyMessage + extractMentions run on the morph path too. 2. If the bot's final PRIVMSG doesn't carry the workflow tag (only a terminal-state TAGMSG is sent), the morph path never fires and the placeholder is stuck on a "Thinking…" spinner while the bot's actual reply lands as a separate row. On terminal-state TAGMSG, remove the still-pending placeholder so the bot's reply can land normally with full reply context.
The tray filters out historical workflows so chathistory replay doesn't pop a wall of cards on channel join, but the inline pill's reopen action was leaving `historical` set -- meaning even after explicit click the card stayed filtered out and nothing happened. Clear it on reopen so an explicit view request always surfaces the card.
The pill used to sit in a flex column to the left of the body, which pushed the entire message content sideways and made the reply look visually off-axis from neighbouring messages. Strip it to a bare clickable icon (step count + name already live on the floating card and history popover) and absolutely-position it in the avatar gutter so the body keeps its natural alignment.
…g from count Two changes to how workflow steps are presented: 1. Display: a tool-call and its matching tool-result now render as a single row -- tool name in the header, then IN (call args) and OUT (result content) stacked, matching the Claude Code "Bash → Commit and push" pattern. Pairing is FIFO by tool name; an orphan result (no preceding call) still renders. Thinking / text rows are unchanged. 2. Counting: countableSteps() now powers the inline pill tooltip and the history popover label. It excludes thinking (model muttering, not work) and counts a tool-call/result pair as one step instead of two.
The auto-scroll effect only fired when the user was already at the bottom, which never happens on initial expand (scrollTop is 0). Track a "has-done-initial-scroll" flag per expansion: force-scroll to the bottom the first time the body mounts/expands, then fall back to the at-bottom check for subsequent step updates so a user scrolled-up to review earlier output isn't yanked away.
…check The previous approach checked "is the user at the bottom?" inside the content-update effect, but by then the new content had already grown scrollHeight -- the old scrollTop no longer qualified as bottom, so we never auto-scrolled even when the user was parked there. Track stickiness via the scroll event instead: whenever the user (or we) scrolls, recompute whether they're at the bottom and stash that into a ref. On content update, honour the flag. Reset to sticky on collapse so a fresh expansion always lands on the latest content.
Adds first-class support for PushBot-style slash commands as defined by
the obbyircd doc/pushbot-spec.md protocol. The user types /forecast
london in #weather and the client:
1. Looks up "forecast" in the per-server botCommands cache (keyed
by lowercased bot nick).
2. If a bot in the current channel exposes that command, builds a
base64-encoded JSON payload { name, options } and sends
@+draft/bot-cmd=<b64> TAGMSG <#channel|botnick>, picking the
PRIVMSG-style "public" or NOTICE-style "private" wire form based
on the command's visibility field.
3. Falls back to the raw IRC command path if no match -- existing
/op, /me etc. behaviour is preserved.
Discovery is event-driven:
* draft/bot-cmds added to ourCaps so the server knows we're aware
of the protocol (informational; the server still does the work).
* registerPushBotHandlers (new src/store/handlers/pushbot.ts) hooks
TAGMSG, decodes +draft/bot-cmds responses, and writes to
server.botCommands. A +draft/bot-cmds-changed broadcast clears
the cached entry so the next slash invocation triggers a refetch.
* The handler is wired into registerAllHandlers in
src/store/handlers/index.ts.
Types extended:
* Server.botCommands: Record<botNick, BotCommand[]> on src/types.
* BotCommand / BotCommandOption mirror the JSON the bot publishes
(name, description, visibility, scopes, options[]).
Resolution order in tryDispatchBotCommand mirrors §7.5 of the spec:
explicit /cmd@botnick targets first, then channel-bots, then DM
partners, then server-wide bots. Public invocations go to the
channel (everyone sees the reply); private ones go to the bot with
+draft/channel-context to keep whisper-style replies routed to the
right view.
Tests: tests/store/pushbot.test.ts covers cache population,
+draft/bot-cmds-changed invalidation, and that unrelated TAGMSGs are
ignored. All 60 test files (789 tests) still pass.
After RPL_ENDOFWHO (315) for a channel, scan the channel's user list for users marked isBot=true (set by handleWhoxReply when the +B mode flag is present) and send a +draft/bot-cmds-query TAGMSG to each that we don't already have cached. Result: by the time the user types '/' in a channel with bots in it, the autocomplete list is already populated.
Two visible-to-the-user gaps now closed: 1. ChatArea rendered the slash popover from `cmdsAvailable` only. Merge in command names from `server.botCommands` so /forecast shows up in the popover when the user is in a channel with a bot that has registered it. 2. Discovery used to fire only on WHO_END. Add a lazy `queryUncachedBotsInChannel` triggered the first time the user starts a slash command in a channel that has +B users without cached schemas, so the popover catches up if WHO completed before the handler attached.
Previously the popover rendered every suggestion as just `/name`; the
caller didn't know whether the source was a server-side built-in or a
PushBot, and there was no signal that /forecast even takes arguments.
Now:
* `SlashSuggestion` carries name + description + options[] + source
({kind:"builtin"} or {kind:"bot", botNick, scope:"channel"|"server"}).
ChatArea builds these from cmdsAvailable (builtins) and from
server.botCommands (PushBot schemas).
* The popover renders the bare name plus a "channel-bot" / "server-bot"
badge with the bot nick, the description below, and `<required>` /
`[optional]` placeholders inline next to the name. Channel-bots
only appear when their bot is a member of the active channel;
server-wide bots show up everywhere we have a cached schema.
* A new SlashParamHint floats above the input once the user has typed
past the command name -- it highlights the active argument (the one
the cursor is currently in), shows the param's type, "required"
tag, description, and any `choices` list. Disappears for builtins
(no schema) and once the user has scrolled past all declared opts.
Tests: 6 new cases for getActiveParamContext covering cmd-name
typing, `//foo` escape, position 0..N argument tracking, `/cmd@bot`
targeting suffix, and case-insensitive cmd matching. All 61 test
files (795 tests) green.
The popover was missing the React-side commands (/me, /msg, /nick, /whisper, /join, /part, /away, /back) because they're handled locally before they touch the wire and never appear in the obsidianirc/cmdslist set. Centralized them in src/lib/clientCommands.ts with full schemas (description + options) so the popover and the param-hint render them identically to PushBot commands. Source kinds now distinguish: * client → handled locally; slate badge, "(handled by ObsidianIRC)" * server → from obsidianirc/cmdslist; emerald badge * bot → draft/bot-cmds; amber "channel-bot" or purple "server-bot" Dedup is client > server > bot, so /me always renders as client even if the server's cmdslist also advertises it. Badges have hover-tooltips explaining the source.
Two passes squashed:
(1) Negotiate the obby.world/channel-bots capability, receive the
server's bot directory burst at welcome time (BATCH wrapper, one
TAGMSG per bot carrying obby.world/bot-info=<base64-json>) plus
incremental add/update/remove pushes. State lands in server.bots
(Record<lowerNick, PushBotInfo>) and mirrors into botCommands so
the slash popover stays warm without a separate
+draft/bot-cmds-query.
(2) New BotsModal — left pane: filter (All / Server-wide / Channel) +
search + scope/status badges and online dot; right pane: realname,
transport, joined channels, command schemas, IRCop action buttons
(Approve / Suspend / Unsuspend / Delete) for non-config-defined
bots. Wired into ChatHeader via onOpenBots, both as a desktop
icon button (🤖, hidden md:block) and as an overflow-menu entry
for narrow/mobile views -- the first cut only added the overflow
entry which is invisible on desktop widths.
Tests: 2 new vitest cases for the bot-info pipeline (add populates
server.bots + botCommands; remove clears both). 61 test files,
797 tests green.
…cons The first cut of BotsModal was a one-off custom layout (flex split, non-portal, ad-hoc styling) using a 🤖 emoji as a header decoration. That worked but it didn't match the rest of the app and looked off on mobile. Rework it to mirror UserSettings: * useMediaQuery to branch desktop vs mobile * useModalBehavior for escape / click-outside * desktop: backdrop + centered card (max-w-4xl, h-80vh) with a fixed- width sidebar (filterable bot list) and a content pane (selected bot detail); Discord-dark palette and discord-primary accents. * mobile: full-screen createPortal with two views (list → detail drill-in, back button to return), safe-area padding matching UserSettings. Icons: every emoji used as UI chrome now uses react-icons/fa. * 🤖 channel-header button → <FaRobot /> * 🤖 overflow-menu entry → <FaRobot /> * online indicator dot in the bot list → <FaCircle /> * empty-state placeholder → <FaRobot className="text-4xl" /> Same surface area: filter chips, search input, status/scope badges, IRCop action buttons (Approve / Suspend / Unsuspend / Delete) for non-config-defined bots. The 🤖 next to bot nicknames in chat is unchanged -- that's a pre-existing bot identity marker, not UI chrome.
…dslist Two bugs: 1) Server-scope bots (helpbot, dicebot) never showed up in the picker. The picker skipped any bot that wasn't a channel member, which was right for channel-scope bots but wrong for server-scope bots that never auto-join. Now: gate the membership check on the bot's scope; server-scope bots are always offered. 2) When a server's cmdsAvailable advertised a name (e.g. HELP) that a bot also defined (helpbot's /help), the server entry won the dedup set and the bot was shadowed; the hint code meanwhile pulled the bot's schema for that name, producing the "picker says it's the built-in but the hint reads like the bot" mismatch. Process bot commands before cmdsAvailable so the canonical bot owner wins the seen-set, and apply the same scope filter to the hint schemas. Drive-by: replace `choices!.length` with `(choices?.length ?? 0)` in SlashParamHint that fix:unsafe had downgraded to an unsafe optional chain on the comparator.
Tauri's TCP/TLS connect path had no timeout, so a stalled handshake to e.g. irc.mirc.club waited on kernel retransmits for ~2 min with no UI feedback. Add 15s timeouts to both the TCP connect and the TLS handshake; the timeout error string flows through socket.onerror, then through serverError, then into a global notification toast. Also add a raw IRC log viewer (Ctrl+Shift+L) that captures every TX/RX line plus connect/error info lines into a 2000-line ring buffer per server. Modal shows the current server's log with copy-all + clear. Android manifest: declare CAMERA/RECORD_AUDIO/MODIFY_AUDIO_SETTINGS plus optional camera/microphone hardware features so Tauri's RustWebChromeClient's runtime permission launcher actually has perms to grant.
…rser Two compounding bugs put mirc.club on ircs://irc.mirc.club:443 instead of :6697 (which is nginx's HTTPS port, hence the HTTP 400 reply): 1. AddServerModal stripped the scheme from the host string before appending the user-entered port, but did NOT strip the embedded :port / path. The discovery prefill passes host="ircs://h:6697", port="6697", so we built ircs://h:6697:6697 — a triple-colon URL. 2. ircUrlParser's fallback (for malformed URLs the regex can't parse) used the WebSocket defaults — 443 for ircs:// and 8000 for irc:// — instead of the IRC standard ports 6697 / 6667. So the malformed triple-colon URL hit the fallback and silently became port 443. Strip everything after the scheme in cleanHost (host part only), and fix the IRC fallback ports. Existing broken entries auto-recover the next time the parser falls through.
Conflicts: - src/components/ui/AddServerModal.tsx: kept this branch's more thorough scheme + port/path strip (regex covers irc:// + path tails, fixing the port-double bug 2c0cf1a was added to address); main's narrower regex was a subset of it. - src/locales/*/messages.mjs: regenerated from the merged .po files via npx lingui compile so they reflect the union of strings.
ValwareIRC
pushed a commit
that referenced
this pull request
Jun 4, 2026
Picked the findings that were genuine correctness or HTML-validity issues; skipped the lingui-extraction nags (those land in a separate i18n pass via lefthook). - BotInvocationChip: switch the inline base64 decode to base64DecodeUtf8 so emoji and non-ASCII bot names round-trip correctly (atob alone is Latin-1). - aiTools.decodeAiToolsValue: reject unknown enum values for workflow state, step type, step state, and action type. The function contract said schema mismatches return null, but obj.state etc. were cast through without checking against the type unions. - ChatArea slash-suggestion fanout: bot scope filter looked up botNick in a lowercased chanUsers set without lowercasing the bot's own nick, so mixed-case bot nicks always reported "not in channel". Also let channel-scoped bots leak into PM context (selectedChannel was used to bypass the membership check); change to require a channel + membership. Both 2410-block and the 2543-block. - BotsModal search filter: guard b.realname with ?? "" so a bot whose RPL_WHOIS lacked the realname field doesn't blow up on toLowerCase. - useMessageSending.sendBotCommand: only dispatch the PM fallback when the command actually declares "pm" in contexts. The old final else fired even for context-restricted commands, sending an invocation the bot would reject anyway. - SlashCommandPopover: the popover is position:fixed so getBoundingClientRect().left is already viewport-relative; the extra + window.scrollX shifted the anchor on scrolled pages. - AiToolsCard header: the outer was a <button> and contained nested Dismiss/Cancel <button>s, which is invalid HTML and reportedly breaks focus/click on some platforms. Convert the outer to a <div role="button"> with keyboard handler; the inner buttons stay native. Biome a11y/useSemanticElements is suppressed at that one spot since using a <button> there is exactly what we're avoiding. - AiToolsCard countdown effect: drop the Zustand action ref `dismiss` from the deps and apply the project's standard biome-ignore for useExhaustiveDependencies (per CLAUDE.md).
|
Automated deployment preview for the PR in the Cloudflare Pages. |
Picked the findings that were genuine correctness or HTML-validity issues; skipped the lingui-extraction nags (those land in a separate i18n pass via lefthook). - BotInvocationChip: switch the inline base64 decode to base64DecodeUtf8 so emoji and non-ASCII bot names round-trip correctly (atob alone is Latin-1). - aiTools.decodeAiToolsValue: reject unknown enum values for workflow state, step type, step state, and action type. The function contract said schema mismatches return null, but obj.state etc. were cast through without checking against the type unions. - ChatArea slash-suggestion fanout: bot scope filter looked up botNick in a lowercased chanUsers set without lowercasing the bot's own nick, so mixed-case bot nicks always reported "not in channel". Also let channel-scoped bots leak into PM context (selectedChannel was used to bypass the membership check); change to require a channel + membership. Both 2410-block and the 2543-block. - BotsModal search filter: guard b.realname with ?? "" so a bot whose RPL_WHOIS lacked the realname field doesn't blow up on toLowerCase. - useMessageSending.sendBotCommand: only dispatch the PM fallback when the command actually declares "pm" in contexts. The old final else fired even for context-restricted commands, sending an invocation the bot would reject anyway. - SlashCommandPopover: the popover is position:fixed so getBoundingClientRect().left is already viewport-relative; the extra + window.scrollX shifted the anchor on scrolled pages. - AiToolsCard header: the outer was a <button> and contained nested Dismiss/Cancel <button>s, which is invalid HTML and reportedly breaks focus/click on some platforms. Convert the outer to a <div role="button"> with keyboard handler; the inner buttons stay native. Biome a11y/useSemanticElements is suppressed at that one spot since using a <button> there is exactly what we're avoiding. - AiToolsCard countdown effect: drop the Zustand action ref `dismiss` from the deps and apply the project's standard biome-ignore for useExhaustiveDependencies (per CLAUDE.md).
ValwareIRC
force-pushed
the
feat/bot-tools-spec
branch
from
cb04983 to
20de3c8
Compare
….lock Matt flagged the build artifacts under android/build/ (lint XML referring to local paths, kotlin compile-cache binaries, manifest-merger logs) and the plugin's Cargo.lock. Remove 148 files (147 from android/build/, 1 Cargo.lock) and add a .gitignore so `tauri android build` doesn't re-track them. Cargo.lock is omitted per Rust library convention (apps commit lockfiles, libraries don't). android/.tauri/ stays tracked to match the ios-keyboard plugin precedent — those are auto-generated tauri-api stubs, not build outputs.
* Rename AiTools -> BotTools across the codebase. The wire format is
draft/bot-tools and matt rightly pointed out the internal naming was
out of sync with the spec. 9 files renamed (src/lib/{aiTools->botTools}
.ts, src/store/handlers/{aiTools->botTools}.ts, the five Ai* components,
both Ai* test files) plus 66+ symbol references (AiTools->BotTools,
aiTools->botTools, AI_TOOLS_->BOT_TOOLS_) updated transitively. Wire-
format strings ("+draft/bot-tools", "draft/bot-tools") stay literal.
* i18n the client slash-command labels in clientCommands.ts. Module-
scope `t` is unsafe (catalogue isn't activated at import time --
CLAUDE.md), so the CLIENT_COMMANDS const becomes a getClientCommands()
function that re-evaluates each call. CLIENT_COMMAND_NAMES stays a
locale-independent Set. 13 new strings translated across all 18
non-English locales in parallel.
* SlashCommandPopover: refuse to render until inputRect has a real
position. Previously the first frame after `isVisible` flipped true
could land before the inputElement ref had resolved, so the popover
rendered at the (100, 100) fallback anchor for one frame -- a visible
flash up-and-left of the input that matt asked about.
The unifiedpush build-artifact cleanup landed in the previous commit.
Matt's right that this doesn't belong here. The whole src-tauri/plugins/unifiedpush/ tree got picked up by accident in fc50a0f ('desktop: surface connect failures...'); my working tree had the parallel feature/webpush branch's scaffolding sitting in the same src-tauri/plugins/ directory and `git add -A` swept it in. Nothing on this branch actually wires the plugin in -- no references in src-tauri/src/, Cargo.toml, tauri.conf.json, or anywhere in src/. It's orphan scaffolding. The plugin lives on feature/webpush (PR #236) where it's actually built into the app. Removed all 30 remaining files (the previous chore commit only got android/build/ + Cargo.lock, 148 files; this clears the rest -- the android/.tauri/ tauri-api stubs and the empty plugin shell). Build + 845 tests still green; nothing on this branch was depending on it.
Resolutions:
- src/lib/irc/IRCClient.ts: both branches added a new CAP REQ entry
in the same neighbourhood (HEAD added "draft/bot-cmds" +
"obby.world/channel-bots"; main added "obby.world/invitation").
Keep all three -- they're independent vendor caps with no overlap.
- src/locales/*.po: took ours then re-extracted to fold in main's new
msgids. Backfilled all 23 invitation-panel strings × 18 locales from
obbyworld/main:src/locales/{loc}/messages.po by copying msgstrs
verbatim (they were translated in PR #231 already). .mjs recompiled.
Build + 858 tests green.
Two CodeRabbit findings, both still valid against the merged tree:
* SlashParamHint left four user-visible literals raw in JSX:
"via @${botNick}", "(handled by ObsidianIRC)", "required", "one of:".
Wrap each with <Trans>, route the bot-nick through Trans's
interpolation so the placeholder reaches the catalogue. Skipped the
"string" type literal (it's a schema-data identifier, not prose) and
the option name/description spans (those are bot-supplied strings,
not translatable from this side).
* fi/messages.po had "offline" -> "offline" (English fallback) on the
BotsModal status badge. Change to "ei verkossa".
Re-extract pulled in the 4 new strings; translated all 18 non-English
locales inline. .mjs recompiled, 858 tests + build green.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/lib/botTools.ts (2)
140-143:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winValidate
featuresagainst the allowed workflow feature set.
typeof f === "string"still accepts unsupported values and then casts them toAiWorkflowFeature[], sodecodeBotToolsValue()can return impossible feature flags even though this decoder is supposed to reject schema mismatches. Filter against aWORKFLOW_FEATURESset here, or returnnullif you want strict rejection.[suggested fix]
Diff
+const WORKFLOW_FEATURES: ReadonlySet<AiWorkflowFeature> = new Set([ + "interactive", + "reasoning", + "approval", +]); + ... - if (Array.isArray(obj.features)) - m.features = obj.features.filter( - (f): f is AiWorkflowFeature => typeof f === "string", - ) as AiWorkflowFeature[]; + if (Array.isArray(obj.features)) { + const features = obj.features.filter( + (f): f is AiWorkflowFeature => + typeof f === "string" && + WORKFLOW_FEATURES.has(f as AiWorkflowFeature), + ); + if (features.length !== obj.features.length) return null; + m.features = features; + }🤖 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 `@src/lib/botTools.ts` around lines 140 - 143, The current filter in decodeBotToolsValue that sets m.features from obj.features only checks typeof f === "string" and then casts to AiWorkflowFeature[], allowing unsupported feature strings; update the filtering to only accept values present in the canonical set (e.g., WORKFLOW_FEATURES) by checking membership before casting, or if you prefer strict decoding, return null from decodeBotToolsValue when any obj.features entry is not in WORKFLOW_FEATURES; target the m.features assignment and use AiWorkflowFeature and WORKFLOW_FEATURES symbols to implement the membership check or early null-return.
150-166:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReject
tool-call/tool-resultframes that omittool.
countableSteps()pairs tool frames bytool, but the decoder currently accepts tool events without a tool name. That lets malformed payloads into state and can undercount by treatingundefined === undefinedas a match. Requireobj.toolwhentypeis"tool-call"or"tool-result".Diff
if ( typeof obj.wid !== "string" || typeof obj.sid !== "string" || typeof obj.type !== "string" || typeof obj.state !== "string" || !STEP_TYPES.has(obj.type as AiStepType) || !STEP_STATES.has(obj.state as AiStepState) ) return null; + if ( + (obj.type === "tool-call" || obj.type === "tool-result") && + typeof obj.tool !== "string" + ) { + return null; + } const m: AiStepMessage = {🤖 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 `@src/lib/botTools.ts` around lines 150 - 166, The decoder that constructs an AiStepMessage currently allows frames with type "tool-call" or "tool-result" to omit obj.tool, which breaks countableSteps() pairing; update the validation in the decoder (the block that checks typeof obj.wid/sid/type/state and builds AiStepMessage) to additionally require typeof obj.tool === "string" and reject (return null) when obj.type is "tool-call" or "tool-result" and obj.tool is missing or not a string, and continue to set m.tool = obj.tool only after that check.
🧹 Nitpick comments (1)
src/lib/clientCommands.ts (1)
10-14: ⚡ Quick winDerive
CLIENT_COMMAND_NAMESfrom the same source asgetClientCommands().Lines 10-14 now understate the update path: the manual
CLIENT_COMMAND_NAMESset adds another sync point beyond the handler branch. If those lists drift, the popover/hint paths that treatgetClientCommands()as canonical will diverge from anyCLIENT_COMMAND_NAMESconsumer. Prefer one locale-independent source of truth for names/scope and derive both exports from it.Also applies to: 124-134
🤖 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 `@src/lib/clientCommands.ts` around lines 10 - 14, Replace the duplicated manual name list with a single source of truth: keep the canonical array/object of client command descriptors (the data used by getClientCommands()) and derive CLIENT_COMMAND_NAMES by mapping that canonical list to its name strings (and similarly derive any other name-only exports around lines 124-134 from the same canonical list); update getClientCommands() to return or filter that canonical descriptor list rather than relying on a separately-maintained set, and remove the hard-coded CLIENT_COMMAND_NAMES so names and scopes always come from the same locale-independent source.
🤖 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.
Inline comments:
In `@src/components/layout/ChatArea.tsx`:
- Around line 951-969: The block that calls fetchUploadInfo(...) and mints
tokens (the tokens array, fetchUploadInfo, ircClient.requestToken and
waitForAuthToken inside the for loop) can throw before per-file try/catch runs;
wrap the entire initialization (the if block guarded by !tokenlessEndpoint &&
filehostUrl) in a try/catch so initialization failures are caught and handled
immediately: on error log it (or surface to the UI) and return early without
starting any per-file jobs, ensuring no unhandled rejection escapes; keep the
existing per-file logic unchanged but rely on the caught initialization error to
prevent further processing.
In `@src/components/ui/AddServerModal.tsx`:
- Around line 119-121: The current host-cleaning logic in AddServerModal
(cleanHost derived from serverHost) uses .replace(/[:/].*$/) which breaks IPv6
addresses; replace that logic by attempting to construct a URL from serverHost
and use url.hostname (which preserves IPv6 bracket notation and removes
port/path), and only fall back to the simple regex-based cleanup if URL parsing
throws; update the code that computes cleanHost in the AddServerModal component
to use this URL-parsing-first approach so ports/paths are stripped correctly
while IPv6 addresses like ::1 or [::1] remain intact.
In `@src/locales/fr/messages.po`:
- Around line 2727-2730: The French translation for the msgid "This server
doesn't support invite links (the<0>obby.world/invitation</0>capability isn't
advertised)..." has no spaces around the inline tag; update the msgstr so there
is a space before the opening tag and a space after the closing tag (i.e.,
change "...la capacité<0>obby.world/invitation</0>n'est..." to "...la capacité
<0>obby.world/invitation</0> n'est...") so the rendered UI text is not
concatenated.
---
Outside diff comments:
In `@src/lib/botTools.ts`:
- Around line 140-143: The current filter in decodeBotToolsValue that sets
m.features from obj.features only checks typeof f === "string" and then casts to
AiWorkflowFeature[], allowing unsupported feature strings; update the filtering
to only accept values present in the canonical set (e.g., WORKFLOW_FEATURES) by
checking membership before casting, or if you prefer strict decoding, return
null from decodeBotToolsValue when any obj.features entry is not in
WORKFLOW_FEATURES; target the m.features assignment and use AiWorkflowFeature
and WORKFLOW_FEATURES symbols to implement the membership check or early
null-return.
- Around line 150-166: The decoder that constructs an AiStepMessage currently
allows frames with type "tool-call" or "tool-result" to omit obj.tool, which
breaks countableSteps() pairing; update the validation in the decoder (the block
that checks typeof obj.wid/sid/type/state and builds AiStepMessage) to
additionally require typeof obj.tool === "string" and reject (return null) when
obj.type is "tool-call" or "tool-result" and obj.tool is missing or not a
string, and continue to set m.tool = obj.tool only after that check.
---
Nitpick comments:
In `@src/lib/clientCommands.ts`:
- Around line 10-14: Replace the duplicated manual name list with a single
source of truth: keep the canonical array/object of client command descriptors
(the data used by getClientCommands()) and derive CLIENT_COMMAND_NAMES by
mapping that canonical list to its name strings (and similarly derive any other
name-only exports around lines 124-134 from the same canonical list); update
getClientCommands() to return or filter that canonical descriptor list rather
than relying on a separately-maintained set, and remove the hard-coded
CLIENT_COMMAND_NAMES so names and scopes always come from the same
locale-independent source.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 99b137ca-58ff-4e85-bbfe-8f61545765ac
📒 Files selected for processing (61)
src/components/layout/ChatArea.tsxsrc/components/layout/ChatHeader.tsxsrc/components/message/BotInvocationChip.tsxsrc/components/message/BotToolsMessagePill.tsxsrc/components/message/BotToolsPlaceholderBody.tsxsrc/components/message/MessageItem.tsxsrc/components/ui/AddServerModal.tsxsrc/components/ui/BotToolsCard.tsxsrc/components/ui/BotToolsHistoryButton.tsxsrc/components/ui/BotToolsTray.tsxsrc/components/ui/BotsModal.tsxsrc/components/ui/SlashCommandPopover.tsxsrc/components/ui/SlashParamHint.tsxsrc/hooks/useMessageSending.tssrc/lib/botTools.tssrc/lib/clientCommands.tssrc/lib/irc/IRCClient.tssrc/locales/cs/messages.mjssrc/locales/cs/messages.posrc/locales/de/messages.mjssrc/locales/de/messages.posrc/locales/en/messages.mjssrc/locales/en/messages.posrc/locales/es/messages.mjssrc/locales/es/messages.posrc/locales/fi/messages.mjssrc/locales/fi/messages.posrc/locales/fr/messages.mjssrc/locales/fr/messages.posrc/locales/it/messages.mjssrc/locales/it/messages.posrc/locales/ja/messages.mjssrc/locales/ja/messages.posrc/locales/ko/messages.mjssrc/locales/ko/messages.posrc/locales/nl/messages.mjssrc/locales/nl/messages.posrc/locales/pl/messages.mjssrc/locales/pl/messages.posrc/locales/pt/messages.mjssrc/locales/pt/messages.posrc/locales/ro/messages.mjssrc/locales/ro/messages.posrc/locales/ru/messages.mjssrc/locales/ru/messages.posrc/locales/sv/messages.mjssrc/locales/sv/messages.posrc/locales/tr/messages.mjssrc/locales/tr/messages.posrc/locales/uk/messages.mjssrc/locales/uk/messages.posrc/locales/zh-TW/messages.mjssrc/locales/zh-TW/messages.posrc/locales/zh/messages.mjssrc/locales/zh/messages.posrc/store/handlers/botTools.tssrc/store/handlers/index.tssrc/store/index.tssrc/types/index.tstests/lib/botTools.test.tstests/store/botTools.test.ts
✅ Files skipped from review due to trivial changes (8)
- src/locales/cs/messages.mjs
- src/locales/es/messages.mjs
- src/locales/es/messages.po
- src/locales/it/messages.po
- src/locales/fi/messages.po
- src/locales/en/messages.po
- src/locales/cs/messages.po
- src/locales/de/messages.po
🚧 Files skipped from review as they are similar to previous changes (11)
- src/lib/irc/IRCClient.ts
- src/locales/en/messages.mjs
- src/locales/de/messages.mjs
- src/locales/it/messages.mjs
- src/components/message/BotInvocationChip.tsx
- src/locales/fr/messages.mjs
- src/components/ui/SlashCommandPopover.tsx
- src/locales/fi/messages.mjs
- src/components/ui/BotsModal.tsx
- src/components/ui/SlashParamHint.tsx
- src/hooks/useMessageSending.ts
Contributor
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/lib/botTools.ts (2)
140-143:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winValidate
featuresagainst the allowed workflow feature set.
typeof f === "string"still accepts unsupported values and then casts them toAiWorkflowFeature[], sodecodeBotToolsValue()can return impossible feature flags even though this decoder is supposed to reject schema mismatches. Filter against aWORKFLOW_FEATURESset here, or returnnullif you want strict rejection.[suggested fix]
Diff
+const WORKFLOW_FEATURES: ReadonlySet<AiWorkflowFeature> = new Set([ + "interactive", + "reasoning", + "approval", +]); + ... - if (Array.isArray(obj.features)) - m.features = obj.features.filter( - (f): f is AiWorkflowFeature => typeof f === "string", - ) as AiWorkflowFeature[]; + if (Array.isArray(obj.features)) { + const features = obj.features.filter( + (f): f is AiWorkflowFeature => + typeof f === "string" && + WORKFLOW_FEATURES.has(f as AiWorkflowFeature), + ); + if (features.length !== obj.features.length) return null; + m.features = features; + }🤖 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 `@src/lib/botTools.ts` around lines 140 - 143, The current filter in decodeBotToolsValue that sets m.features from obj.features only checks typeof f === "string" and then casts to AiWorkflowFeature[], allowing unsupported feature strings; update the filtering to only accept values present in the canonical set (e.g., WORKFLOW_FEATURES) by checking membership before casting, or if you prefer strict decoding, return null from decodeBotToolsValue when any obj.features entry is not in WORKFLOW_FEATURES; target the m.features assignment and use AiWorkflowFeature and WORKFLOW_FEATURES symbols to implement the membership check or early null-return.
150-166:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReject
tool-call/tool-resultframes that omittool.
countableSteps()pairs tool frames bytool, but the decoder currently accepts tool events without a tool name. That lets malformed payloads into state and can undercount by treatingundefined === undefinedas a match. Requireobj.toolwhentypeis"tool-call"or"tool-result".Diff
if ( typeof obj.wid !== "string" || typeof obj.sid !== "string" || typeof obj.type !== "string" || typeof obj.state !== "string" || !STEP_TYPES.has(obj.type as AiStepType) || !STEP_STATES.has(obj.state as AiStepState) ) return null; + if ( + (obj.type === "tool-call" || obj.type === "tool-result") && + typeof obj.tool !== "string" + ) { + return null; + } const m: AiStepMessage = {🤖 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 `@src/lib/botTools.ts` around lines 150 - 166, The decoder that constructs an AiStepMessage currently allows frames with type "tool-call" or "tool-result" to omit obj.tool, which breaks countableSteps() pairing; update the validation in the decoder (the block that checks typeof obj.wid/sid/type/state and builds AiStepMessage) to additionally require typeof obj.tool === "string" and reject (return null) when obj.type is "tool-call" or "tool-result" and obj.tool is missing or not a string, and continue to set m.tool = obj.tool only after that check.
🧹 Nitpick comments (1)
src/lib/clientCommands.ts (1)
10-14: ⚡ Quick winDerive
CLIENT_COMMAND_NAMESfrom the same source asgetClientCommands().Lines 10-14 now understate the update path: the manual
CLIENT_COMMAND_NAMESset adds another sync point beyond the handler branch. If those lists drift, the popover/hint paths that treatgetClientCommands()as canonical will diverge from anyCLIENT_COMMAND_NAMESconsumer. Prefer one locale-independent source of truth for names/scope and derive both exports from it.Also applies to: 124-134
🤖 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 `@src/lib/clientCommands.ts` around lines 10 - 14, Replace the duplicated manual name list with a single source of truth: keep the canonical array/object of client command descriptors (the data used by getClientCommands()) and derive CLIENT_COMMAND_NAMES by mapping that canonical list to its name strings (and similarly derive any other name-only exports around lines 124-134 from the same canonical list); update getClientCommands() to return or filter that canonical descriptor list rather than relying on a separately-maintained set, and remove the hard-coded CLIENT_COMMAND_NAMES so names and scopes always come from the same locale-independent source.
🤖 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.
Inline comments:
In `@src/components/layout/ChatArea.tsx`:
- Around line 951-969: The block that calls fetchUploadInfo(...) and mints
tokens (the tokens array, fetchUploadInfo, ircClient.requestToken and
waitForAuthToken inside the for loop) can throw before per-file try/catch runs;
wrap the entire initialization (the if block guarded by !tokenlessEndpoint &&
filehostUrl) in a try/catch so initialization failures are caught and handled
immediately: on error log it (or surface to the UI) and return early without
starting any per-file jobs, ensuring no unhandled rejection escapes; keep the
existing per-file logic unchanged but rely on the caught initialization error to
prevent further processing.
In `@src/components/ui/AddServerModal.tsx`:
- Around line 119-121: The current host-cleaning logic in AddServerModal
(cleanHost derived from serverHost) uses .replace(/[:/].*$/) which breaks IPv6
addresses; replace that logic by attempting to construct a URL from serverHost
and use url.hostname (which preserves IPv6 bracket notation and removes
port/path), and only fall back to the simple regex-based cleanup if URL parsing
throws; update the code that computes cleanHost in the AddServerModal component
to use this URL-parsing-first approach so ports/paths are stripped correctly
while IPv6 addresses like ::1 or [::1] remain intact.
In `@src/locales/fr/messages.po`:
- Around line 2727-2730: The French translation for the msgid "This server
doesn't support invite links (the<0>obby.world/invitation</0>capability isn't
advertised)..." has no spaces around the inline tag; update the msgstr so there
is a space before the opening tag and a space after the closing tag (i.e.,
change "...la capacité<0>obby.world/invitation</0>n'est..." to "...la capacité
<0>obby.world/invitation</0> n'est...") so the rendered UI text is not
concatenated.
---
Outside diff comments:
In `@src/lib/botTools.ts`:
- Around line 140-143: The current filter in decodeBotToolsValue that sets
m.features from obj.features only checks typeof f === "string" and then casts to
AiWorkflowFeature[], allowing unsupported feature strings; update the filtering
to only accept values present in the canonical set (e.g., WORKFLOW_FEATURES) by
checking membership before casting, or if you prefer strict decoding, return
null from decodeBotToolsValue when any obj.features entry is not in
WORKFLOW_FEATURES; target the m.features assignment and use AiWorkflowFeature
and WORKFLOW_FEATURES symbols to implement the membership check or early
null-return.
- Around line 150-166: The decoder that constructs an AiStepMessage currently
allows frames with type "tool-call" or "tool-result" to omit obj.tool, which
breaks countableSteps() pairing; update the validation in the decoder (the block
that checks typeof obj.wid/sid/type/state and builds AiStepMessage) to
additionally require typeof obj.tool === "string" and reject (return null) when
obj.type is "tool-call" or "tool-result" and obj.tool is missing or not a
string, and continue to set m.tool = obj.tool only after that check.
---
Nitpick comments:
In `@src/lib/clientCommands.ts`:
- Around line 10-14: Replace the duplicated manual name list with a single
source of truth: keep the canonical array/object of client command descriptors
(the data used by getClientCommands()) and derive CLIENT_COMMAND_NAMES by
mapping that canonical list to its name strings (and similarly derive any other
name-only exports around lines 124-134 from the same canonical list); update
getClientCommands() to return or filter that canonical descriptor list rather
than relying on a separately-maintained set, and remove the hard-coded
CLIENT_COMMAND_NAMES so names and scopes always come from the same
locale-independent source.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 99b137ca-58ff-4e85-bbfe-8f61545765ac
📒 Files selected for processing (61)
src/components/layout/ChatArea.tsxsrc/components/layout/ChatHeader.tsxsrc/components/message/BotInvocationChip.tsxsrc/components/message/BotToolsMessagePill.tsxsrc/components/message/BotToolsPlaceholderBody.tsxsrc/components/message/MessageItem.tsxsrc/components/ui/AddServerModal.tsxsrc/components/ui/BotToolsCard.tsxsrc/components/ui/BotToolsHistoryButton.tsxsrc/components/ui/BotToolsTray.tsxsrc/components/ui/BotsModal.tsxsrc/components/ui/SlashCommandPopover.tsxsrc/components/ui/SlashParamHint.tsxsrc/hooks/useMessageSending.tssrc/lib/botTools.tssrc/lib/clientCommands.tssrc/lib/irc/IRCClient.tssrc/locales/cs/messages.mjssrc/locales/cs/messages.posrc/locales/de/messages.mjssrc/locales/de/messages.posrc/locales/en/messages.mjssrc/locales/en/messages.posrc/locales/es/messages.mjssrc/locales/es/messages.posrc/locales/fi/messages.mjssrc/locales/fi/messages.posrc/locales/fr/messages.mjssrc/locales/fr/messages.posrc/locales/it/messages.mjssrc/locales/it/messages.posrc/locales/ja/messages.mjssrc/locales/ja/messages.posrc/locales/ko/messages.mjssrc/locales/ko/messages.posrc/locales/nl/messages.mjssrc/locales/nl/messages.posrc/locales/pl/messages.mjssrc/locales/pl/messages.posrc/locales/pt/messages.mjssrc/locales/pt/messages.posrc/locales/ro/messages.mjssrc/locales/ro/messages.posrc/locales/ru/messages.mjssrc/locales/ru/messages.posrc/locales/sv/messages.mjssrc/locales/sv/messages.posrc/locales/tr/messages.mjssrc/locales/tr/messages.posrc/locales/uk/messages.mjssrc/locales/uk/messages.posrc/locales/zh-TW/messages.mjssrc/locales/zh-TW/messages.posrc/locales/zh/messages.mjssrc/locales/zh/messages.posrc/store/handlers/botTools.tssrc/store/handlers/index.tssrc/store/index.tssrc/types/index.tstests/lib/botTools.test.tstests/store/botTools.test.ts
✅ Files skipped from review due to trivial changes (8)
- src/locales/cs/messages.mjs
- src/locales/es/messages.mjs
- src/locales/es/messages.po
- src/locales/it/messages.po
- src/locales/fi/messages.po
- src/locales/en/messages.po
- src/locales/cs/messages.po
- src/locales/de/messages.po
🚧 Files skipped from review as they are similar to previous changes (11)
- src/lib/irc/IRCClient.ts
- src/locales/en/messages.mjs
- src/locales/de/messages.mjs
- src/locales/it/messages.mjs
- src/components/message/BotInvocationChip.tsx
- src/locales/fr/messages.mjs
- src/components/ui/SlashCommandPopover.tsx
- src/locales/fi/messages.mjs
- src/components/ui/BotsModal.tsx
- src/components/ui/SlashParamHint.tsx
- src/hooks/useMessageSending.ts
🛑 Comments failed to post (3)
src/components/layout/ChatArea.tsx (1)
951-969:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winCatch upload-initialization failures before starting jobs.
fetchUploadInfo/ token minting can throw before per-file upload try/catch runs. Right now that failure can escape UI handlers as an unhandled rejection and fail silently for users.🛠️ Proposed fix
- if (!tokenlessEndpoint && filehostUrl) { - info = await fetchUploadInfo(filehostUrl); - // Serialised because waitForAuthToken resolves on the first matching - // TOKEN_GENERATE event -- parallel mints would race for the reply. - const scope = target.startsWith("#") ? `channel:${target}` : undefined; - for (let i = 0; i < files.length; i++) { - ircClient.requestToken(selectedServerId, "filehost", scope); - const tok = await waitForAuthToken(selectedServerId, "filehost"); - if (!tok) { - console.error( - "draft/authtoken: server did not return a filehost token", - ); - return; - } - tokens.push(tok); - } - } + try { + if (!tokenlessEndpoint && filehostUrl) { + info = await fetchUploadInfo(filehostUrl); + // Serialised because waitForAuthToken resolves on the first matching + // TOKEN_GENERATE event -- parallel mints would race for the reply. + const scope = target.startsWith("#") ? `channel:${target}` : undefined; + for (let i = 0; i < files.length; i++) { + ircClient.requestToken(selectedServerId, "filehost", scope); + const tok = await waitForAuthToken(selectedServerId, "filehost"); + if (!tok) { + console.error( + "draft/authtoken: server did not return a filehost token", + ); + return; + } + tokens.push(tok); + } + } + } catch (err) { + console.error("File upload initialization failed:", err); + return; + }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.let info: Awaited<ReturnType<typeof fetchUploadInfo>> = null; const tokens: string[] = []; try { if (!tokenlessEndpoint && filehostUrl) { info = await fetchUploadInfo(filehostUrl); // Serialised because waitForAuthToken resolves on the first matching // TOKEN_GENERATE event -- parallel mints would race for the reply. const scope = target.startsWith("#") ? `channel:${target}` : undefined; for (let i = 0; i < files.length; i++) { ircClient.requestToken(selectedServerId, "filehost", scope); const tok = await waitForAuthToken(selectedServerId, "filehost"); if (!tok) { console.error( "draft/authtoken: server did not return a filehost token", ); return; } tokens.push(tok); } } } catch (err) { console.error("File upload initialization failed:", err); return; }🤖 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 `@src/components/layout/ChatArea.tsx` around lines 951 - 969, The block that calls fetchUploadInfo(...) and mints tokens (the tokens array, fetchUploadInfo, ircClient.requestToken and waitForAuthToken inside the for loop) can throw before per-file try/catch runs; wrap the entire initialization (the if block guarded by !tokenlessEndpoint && filehostUrl) in a try/catch so initialization failures are caught and handled immediately: on error log it (or surface to the UI) and return early without starting any per-file jobs, ensuring no unhandled rejection escapes; keep the existing per-file logic unchanged but rely on the caught initialization error to prevent further processing.src/components/ui/AddServerModal.tsx (1)
119-121:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winIPv6 address parsing is broken by the regex.
The regex
/[:/].*$/strips everything from the first:or/, which breaks IPv6 addresses:
[::1]→[(colon inside brackets is matched)::1→:(first colon is matched)[2001:db8::1]→[2001This will cause connection failures if users enter IPv6 addresses (e.g., localhost
::1for development).🔧 Proposed fix using URL parsing
Use URL parsing to extract the hostname, which correctly handles IPv6 bracket notation:
const port = Number.parseInt(serverPort, 10); - // Strip scheme AND any embedded :port / path so we don't end up - // appending port twice (e.g. ircs://host:6697:6697). - const cleanHost = serverHost - .replace(/^(https?|wss?|ircs?|irc):\/\//, "") - .replace(/[:/].*$/, ""); + // Strip scheme and extract hostname (handles IPv6, ports, and paths) + let cleanHost = serverHost.replace(/^(https?|wss?|ircs?|irc):\/\//, ""); + try { + // Use URL parsing to extract hostname (handles IPv6 brackets) + const url = new URL(`http://${cleanHost}`); + cleanHost = url.hostname; + } catch { + // Fall back to regex for plain hostnames without special chars + cleanHost = cleanHost.replace(/[:/].*$/, ""); + } finalHost = useWebSocket ? `wss://${cleanHost}:${port}` : `ircs://${cleanHost}:${port}`;This approach:
- Uses
URL.hostnamewhich correctly extracts IPv6 addresses from bracket notation- Falls back to the existing regex for simple hostnames if URL parsing fails
- Preserves the intent to strip ports and paths while fixing IPv6 support
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.const port = Number.parseInt(serverPort, 10); // Strip scheme and extract hostname (handles IPv6, ports, and paths) let cleanHost = serverHost.replace(/^(https?|wss?|ircs?|irc):\/\//, ""); try { // Use URL parsing to extract hostname (handles IPv6 brackets) const url = new URL(`http://${cleanHost}`); cleanHost = url.hostname; } catch { // Fall back to regex for plain hostnames without special chars cleanHost = cleanHost.replace(/[:/].*$/, ""); } finalHost = useWebSocket ? `wss://${cleanHost}:${port}` : `ircs://${cleanHost}:${port}`;🤖 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 `@src/components/ui/AddServerModal.tsx` around lines 119 - 121, The current host-cleaning logic in AddServerModal (cleanHost derived from serverHost) uses .replace(/[:/].*$/) which breaks IPv6 addresses; replace that logic by attempting to construct a URL from serverHost and use url.hostname (which preserves IPv6 bracket notation and removes port/path), and only fall back to the simple regex-based cleanup if URL parsing throws; update the code that computes cleanHost in the AddServerModal component to use this URL-parsing-first approach so ports/paths are stripped correctly while IPv6 addresses like ::1 or [::1] remain intact.src/locales/fr/messages.po (1)
2727-2730:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd spaces around the inline capability tag in the French translation.
Line 2729 currently concatenates words around
<0>...</0>, which will render awkwardly in UI text.💡 Suggested fix
-msgstr "Ce serveur ne prend pas en charge les liens d'invitation (la capacité<0>obby.world/invitation</0>n'est pas annoncée). Vous pouvez toujours discuter normalement ; ce panneau est destiné aux réseaux propulsés par obbyircd." +msgstr "Ce serveur ne prend pas en charge les liens d'invitation (la capacité <0>obby.world/invitation</0> n'est pas annoncée). Vous pouvez toujours discuter normalement ; ce panneau est destiné aux réseaux propulsés par obbyircd."🤖 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 `@src/locales/fr/messages.po` around lines 2727 - 2730, The French translation for the msgid "This server doesn't support invite links (the<0>obby.world/invitation</0>capability isn't advertised)..." has no spaces around the inline tag; update the msgstr so there is a space before the opening tag and a space after the closing tag (i.e., change "...la capacité<0>obby.world/invitation</0>n'est..." to "...la capacité <0>obby.world/invitation</0> n'est...") so the rendered UI text is not concatenated.
ValwareIRC
requested review from
matheusfillipe
and removed request for
matheusfillipe
…y rename - pushbot handler: on BATCH_START with type=draft/bot-cmds, allocate a fragments buffer keyed by (serverId, batchRef). Each in-batch TAGMSG with +draft/bot-cmds appends its raw base64 fragment to the buffer instead of decoding individually. On BATCH_END, concatenate, base64-decode, JSON-parse, and commit. Matches the bot-tools spec's 'split across batch messages, concatenated before decoding' shape. - Rename +obby.world/invoked-by -> +draft/invoked-by in the bot invocation chip and the MessageItem tag lookup. The spec now defines this tag under draft/.
…y bot The draft/bot-cmds spec uses BATCH +ref draft/bot-cmds <target>, where the target on the open line is the asker, not the sender. The sender nick lives in the BATCH command's source prefix; without exposing it on the BATCH_START event the pushbot handler stored each batch under an empty-string key and could never write the reassembled list back to the right bot's botCommands entry.
matheusfillipe
left a comment
matheusfillipe
left a comment
Contributor
There was a problem hiding this comment.
did a local review, flagging the few that actually worry me. the bot-cmd shadowing one is the important one.
| } | ||
| // server-wide bots (any bot we know that defines the command) | ||
| if (!matches.length) { | ||
| for (const [bot, list] of Object.entries(bots)) { |
Contributor
There was a problem hiding this comment.
security: this server-wide fallback lets a bot shadow real commands. /oper x y and /ns identify pass aren't builtins so they fall through to here before the raw send, and we match any bot anywhere on the network. so a bot registering oper/ns just grabs the user's password. we also cache botCommands per sender with no isBot check (commitBotCmds). think we should only dispatch on explicit /cmd@bot or isBot + in-channel, kill the server-wide fallback, and never let a bot name shadow a server/privileged command.
| processedMessageIds, | ||
| }; | ||
| } | ||
| return { aiWorkflows, processedMessageIds }; |
Contributor
There was a problem hiding this comment.
i think we drop the bot's final answer here when there was no placeholder. on idx < 0 we still add the msgid to processedMessageIds and return without pushing a row, and since botTools runs before the message handler the normal CHANMSG path then dedupes it away. happens when a bot doesn't tag start or you join mid-workflow. on idx < 0 we should just append it as a normal message instead of swallowing it.
| )} | ||
|
|
||
| <div className="relative min-w-0"> | ||
| <BotInvocationChip tagValue={message.tags?.["+draft/invoked-by"]} /> |
Contributor
There was a problem hiding this comment.
this renders for any message carrying +draft/invoked-by, and it's a + client tag so anyone can forge it. i can send a normal line tagged invoked-by={nick:"someadmin",name:"ban"...} and everyone in the channel sees 'someadmin ran /ban ...'. should only show it for real bot senders (we already have isBot right here) and not trust the nick blindly.
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.
Reworks and combines two previously-separate feature branches — the workflow viewer (
draft-ai-tools) and slash commands (feat/pushbot-client) — to the IRCv3 Bot Tools draft spec.draft/bot-tools (workflow transparency)
draft/bot-tools(wasdraft/ai-tools); client-only tag+draft/bot-tools(was the vendor+obby.world/ai-tools)src/lib/base64.ts) instead of escaped raw JSONthinking→reasoning; thesteeraction →input; added the workflowfeaturesarray (interactive/reasoning/approval) and stepcancelled-bydraft/bot-cmds (slash commands)
+draft/bot-cmds-query(was=1)contexts(public/private/pm) +requires, dropping the legacyvisibility/scopes/versionfieldsbot(public-channel disambiguation) and thechannel(private context) instead of relying on+draft/channel-context, which is not valid onTAGMSGThe obby.world bot-directory layer (
obby.world/channel-bots,obby.world/bot-info, the Bots management modal) is intentionally kept as a vendor feature — only its command display now readsdraft/bot-cmds.Testing
npm run test— 818 passing, 1 skippednpm run build— cleanServer side
Requires the companion obbyircd changes on
unreal60_dev(commit11d5d6d76): thedraft/bot-tools/draft/bot-cmdscapabilities, schema normalisation,+draft/bot-cmd-error, and the legacy-compatibility bridge.Summary by CodeRabbit