-
Notifications
You must be signed in to change notification settings - Fork 0
Hot-reload agent-authored skills and subagents #22
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: dev
Are you sure you want to change the base?
Conversation
Summary of ChangesHello @IgorWarzocha, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a significant quality-of-life improvement for developers and agents alike by implementing a hot-reloading capability. It allows for dynamic updates of agent configurations, skills, and commands without requiring a full application restart. This feature streamlines the development and iteration process, making it more fluid and responsive, particularly for agent-driven development workflows. The changes are carefully isolated behind an experimental flag to maintain stability. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces an experimental hot-reloading feature for agents, skills, and commands, which is a great addition for faster development cycles. The implementation correctly uses file watchers and instance state invalidation. However, there are several areas for improvement, primarily concerning unhandled promises which could lead to silent failures, and duplicated code for path checking which impacts maintainability. I've also noted a missing watcher initialization for custom tools. Addressing these points will make the feature more robust and easier to maintain.
| case "config.updated": { | ||
| const sdk = createClient(directory) | ||
| Promise.all([ | ||
| sdk.app.agents().then((x) => setStore("agent", x.data ?? [])), | ||
| sdk.command.list().then((x) => setStore("command", x.data ?? [])), | ||
| sdk.config.get().then((x) => setStore("config", x.data!)), | ||
| ]) | ||
| break | ||
| } | ||
| case "command.updated": { | ||
| const sdk = createClient(directory) | ||
| sdk.command.list().then((x) => setStore("command", x.data ?? [])) | ||
| break | ||
| } | ||
| case "skill.updated": { | ||
| const sdk = createClient(directory) | ||
| sdk.app.agents().then((x) => setStore("agent", x.data ?? [])) | ||
| break | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The promises returned by the SDK calls in the config.updated, command.updated, and skill.updated event handlers are not handled with .catch(). This can lead to unhandled promise rejections if an API call fails, causing silent failures in state updates. Additionally, in the config.updated handler, the non-null assertion x.data! is unsafe and could cause a runtime crash. Please add error handling to these promises and replace the non-null assertion with a safe access pattern.
| if (hasCommand) { | ||
| refresh.push(sdk.command.list().then((x) => setStore("command", x.data ?? []))) | ||
| } | ||
| Promise.all(refresh) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Promise.all(refresh) call is not awaited and lacks a .catch() handler. If any of the promises in the refresh array reject (e.g., due to a network error), it will result in an unhandled promise rejection. This could cause the hot-reload to fail silently. It's safer to handle potential errors.
Promise.all(refresh).catch(err => console.error("Error during hot-reload refresh:", err));
| const configRoot = Global.Path.config.replaceAll("\\", "/") | ||
| const configDirs = await Config.directories() | ||
| const normalizedDirs = configDirs.map((dir) => dir.replaceAll("\\", "/")) | ||
| const looksLikeConfigDir = | ||
| filepath.includes("/.opencode/") || | ||
| filepath.startsWith(".opencode/") || | ||
| filepath.includes("/.config/opencode/") || | ||
| filepath.startsWith(".config/opencode/") | ||
| const inConfigDir = | ||
| looksLikeConfigDir || | ||
| filepath === configRoot || | ||
| filepath.startsWith(configRoot + "/") || | ||
| normalizedDirs.some((dir) => Filesystem.contains(dir, filepath) || filepath === dir) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This logic for determining if a file is within a configuration directory is duplicated in packages/opencode/src/command/index.ts, packages/opencode/src/config/config.ts, and packages/opencode/src/tool/registry.ts. To improve maintainability and reduce redundancy, consider extracting this into a shared helper function, for example in packages/opencode/src/config/config.ts.
| if (hasCommand) { | ||
| refresh.push(sdk.client.command.list().then((x) => setStore("command", reconcile(x.data ?? [])))) | ||
| } | ||
| Promise.all(refresh) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Promise.all(refresh) call is not awaited and lacks a .catch() handler. If any of the promises reject, it will result in an unhandled promise rejection, causing the TUI to not update on file changes without any visible error. Please handle potential errors.
Promise.all(refresh).catch(err => console.error("Error during TUI hot-reload refresh:", err));
| case "config.updated": { | ||
| setStore("config", reconcile(event.properties)) | ||
| sdk.client.app.agents().then((x) => setStore("agent", reconcile(x.data ?? []))) | ||
| sdk.client.command.list().then((x) => setStore("command", reconcile(x.data ?? []))) | ||
| break | ||
| } | ||
|
|
||
| case "command.updated": { | ||
| sdk.client.command.list().then((x) => setStore("command", reconcile(x.data ?? []))) | ||
| break | ||
| } | ||
|
|
||
| case "skill.updated": { | ||
| sdk.client.app.agents().then((x) => setStore("agent", reconcile(x.data ?? []))) | ||
| break | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the new event handlers for config.updated, command.updated, and skill.updated, the promises returned by the SDK calls (e.g., sdk.client.command.list()) are not being handled with .catch(). This can lead to unhandled promise rejections if an API call fails, which might cause silent failures in the TUI state updates. Please add error handling to these promises.
| if (Flag.OPENCODE_EXPERIMENTAL_HOT_RELOAD()) { | ||
| Config.initWatcher() | ||
| Command.initWatcher() | ||
| Agent.initWatcher() | ||
| Skill.initWatcher() | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The initWatcher functions for Config, Command, Agent, and Skill are correctly initialized here for hot-reloading. However, ToolRegistry.initWatcher() is also defined but is not being called. This means custom tools (.ts/.js files) will not be hot-reloaded as intended. Please add the call to ToolRegistry.initWatcher() here.
| if (Flag.OPENCODE_EXPERIMENTAL_HOT_RELOAD()) { | |
| Config.initWatcher() | |
| Command.initWatcher() | |
| Agent.initWatcher() | |
| Skill.initWatcher() | |
| } | |
| if (Flag.OPENCODE_EXPERIMENTAL_HOT_RELOAD()) { | |
| Config.initWatcher() | |
| Command.initWatcher() | |
| Agent.initWatcher() | |
| Skill.initWatcher() | |
| ToolRegistry.initWatcher() | |
| } |
|
Neat! Big PR, but if it works well, I'd be into it. I'll try to give it a try this evening. |
Allow configs to be invalidated and reloaded while OC is running. This not only enables the users to create new agents/skills/commands and instantly use them, but it also allows the agents themselves to develop these skills or create new agents on the fly and instantly use them (Ralph Loops, anyone?).
Proposal:
Watch instance + config directories when hot-reload is enabled.
When agent/, skill/, or command/ markdown changes: invalidate cached state and reload immediately.
When custom tool .ts/.js changes: re-import with cache busting. (I believe this is a side effect that can be removed)
Emit config.updated, skill.updated, command.updated so TUI/Desktop refresh live.
Is it safe? Not necessarily, and is not for everyone. I have a functioning version of it, that is locked behind EXPERIMENTAL_HOT_RELOAD flag. Kept changes to minimum and preserved everything from upstream. https://github.com/IgorWarzocha/opencode/tree/agent-skills-commands-hot-reloading
It works, but there are some caveats - agents would need a create skill/command/agent tool for creating a markdown files that pass the parsing check (if it fails, nothing happens). This is probably the easiest implementation - let agent do the thing, and as a return of the tool call, give it a path to the file and ask it to expand the instructions set and leave the frontmatter alone.
Toast on reload would be a nice addition, but I'm not doing either until I know if there's actually a demand for this.