fix: improve plugin system robustness — agent/command resolution, async errors, hook timing, two-phase init#18280
Open
ryanskidmore wants to merge 1 commit intoanomalyco:devfrom
Open
Conversation
Contributor
|
Thanks for updating your PR! It now meets our contributing guidelines. 👍 |
d8866cb to
cc07261
Compare
…nc error handling, hook timing, and two-phase init - Guard against undefined Agent.get()/Command.get() at all call sites with typed NamedError and available-name hints instead of silent TypeErrors - Add .catch() on prompt_async route to surface detached prompt failures via Session.Event.Error instead of silently swallowing them - Isolate plugin config hook errors with per-hook try/catch so one failing hook no longer kills the entire bootstrap - Move command.execute.before hook to fire before template parts are merged with input parts, giving plugins access to raw template content - Add two-phase plugin initialization so plugins injected by config hooks are loaded and initialized in the same startup pass - Add tests for agent/command resolution errors, async error handling, command hook timing, and plugin config hook ordering
cc07261 to
fef1c42
Compare
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
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.
Issue for this PR
Closes #18310
Type of change
What does this PR do?
Agent.get()andCommand.get()returnundefinedfor unknown names, but five call sites inprompt.tsdereference the result without null checks — producing a rawTypeErrorinstead of a useful error. Theprompt_asyncroute drops errors silently because the detachedSessionPrompt.prompt()call has no.catch(). Pluginconfighooks aren't error-isolated, so one throwing hook kills the rest. Thecommand.execute.beforehook fires after template parts are merged with caller parts, making clean template replacement fragile. And plugins added by a config hook aren't loaded in the same startup pass becausePlugin.state()is already cached.Fixes:
Agent/command null guards — Added null checks at all five
Agent.get()/Command.get()call sites (createUserMessage,loopnormal + subtask,shell,command). On missing agent/command, throwsNamedError.Unknownwith the list of available names and publishesSession.Event.Error. Follows the existing guard pattern already at line 1858 ofprompt.ts. Empty-string agent falls back toAgent.defaultAgent()instead of looking up"".prompt_async error handling — Added
.catch()on the detachedSessionPrompt.prompt()call in theprompt_asyncroute that logs the error and publishesSession.Event.Error. The route still returns 204 immediately — the fire-and-forget semantics are preserved, errors are just no longer invisible.Config hook error isolation — Wrapped each individual
hook.config?.()call inPlugin.init()with try/catch so a failing hook is logged but doesn't prevent others from running.Command hook timing — Moved
Plugin.trigger("command.execute.before")to fire before template parts are merged withinput.parts. Plugins now receive template-only parts inoutput.partsand can replace template content cleanly.input.partsare merged after the hook returns. Subtask path unchanged. No hook type signature change.Two-phase plugin init — After config hooks fire in
Plugin.init(), the code now diffsconfig.pluginagainst what was originally loaded, imports any newly-added plugins using the same install/import path asstate(), runs their config hooks once, and pushes them into the sharedhooksarray. One extra pass only — no recursive loading.How did you verify your code works?
7 new test cases added to existing test files, all passing:
test/session/prompt.test.ts— unknown agent throwsNamedError.Unknown(notTypeError), error includes available names, empty agent falls back to default, unknown command throws with names; command hook receives template-only parts before input mergetest/server/session-messages.test.ts— structural test verifying.catch()+Bus.publish(Session.Event.Error)onprompt_asyncroutetest/plugin/auth-override.test.ts— two-phase plugin loading structure verified, config hooks individually error-isolated with try/catchbun typecheckclean across all 13 packages.bun testpasses.Screenshots / recordings
N/A — no UI changes.
Checklist