feat: implement hot-reload for agents, skills, and config #21
Conversation
- Added OPENCODE_EXPERIMENTAL_HOT_RELOAD_AGENTIC and OPENCODE_EXPERIMENTAL_HOT_RELOAD_CONFIG flags - Extended FileWatcher to support watching config directories with context preservation - Implemented initWatcher logic for Config, Skill, Agent, Command, and ToolRegistry - Added TUI and App event handlers for live updates - Enhanced instance context management for async watcher callbacks - Added tests for config and skill hot reloading
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 delivers a significant enhancement by integrating hot-reload capabilities across key components of the application, including agents, skills, and configuration. The primary goal is to streamline development and improve operational efficiency by enabling real-time application of changes without manual restarts. This involves foundational changes to the file watching mechanism, event bus, and component initialization, ensuring a responsive and dynamic user experience. 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
|
|
@codex review this PR. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a significant and well-planned hot-reload feature for agents, skills, and configurations. The implementation is comprehensive, with changes spanning both backend and frontend components, and the use of experimental flags provides a good safety net. The refactoring of the file watcher and instance context management is a solid improvement. However, I've identified a few high-severity issues related to incorrect state invalidation in the hot-reload logic and a potential runtime error in the TUI. There are also some minor type safety concerns that should be addressed. Overall, this is a valuable addition that will greatly improve the development workflow.
| setAgentStore("current", agents()[0].name) | ||
| return agents()[0] | ||
| } | ||
| return agents()[0]! |
There was a problem hiding this comment.
There's a potential runtime error here. If agents() returns an empty array, the if (agents().length > 0) block is skipped, and this line will be executed. At this point, agents()[0] is undefined, and using the non-null assertion ! will cause the application to crash. You should handle the case where no agents are available more gracefully, for example by returning undefined and letting the consumer of this function handle it.
return undefined
| try { | ||
| // Invalidate and reload state | ||
| const result = await state() | ||
| Bus.publish(Events.Updated, result.config) | ||
| log.info("config reloaded successfully") | ||
| } catch (err) { | ||
| log.error("failed to reload config", { error: err }) | ||
| } |
There was a problem hiding this comment.
The hot-reload logic for config appears to be incorrect. Simply calling await state() will return the cached configuration because the state is not being invalidated. This will prevent the hot-reload from actually taking effect.
You should refactor this to use Instance.invalidate before reloading the state, following the pattern used in packages/opencode/src/command/index.ts.
- Extract the async initializer function from
Instance.stateinto a named function (e.g.,initState). - Change the
statedefinition toexport const state = Instance.state(initState);. - In
initWatcher, callawait Instance.invalidate(initState)before fetching the new state.
packages/opencode/src/skill/skill.ts
Outdated
| try { | ||
| // Reload skills and emit update event | ||
| await state() | ||
| Bus.publish(Events.Updated, {}) | ||
| log.info("skills reloaded successfully") | ||
| } catch (err) { | ||
| log.error("failed to reload skills", { error: err }) | ||
| } |
There was a problem hiding this comment.
This hot-reload logic has the same issue as the one in config.ts. Calling await state() will return the cached list of skills without reloading them from the filesystem because the state cache is not being invalidated.
To fix this, please apply the same initState and Instance.invalidate pattern used in other parts of the codebase (like command/index.ts):
- Extract the async initializer from
Instance.stateinto a namedinitStatefunction. - Define
export const state = Instance.state(initState);. - In
initWatcher, callawait Instance.invalidate(initState)beforeawait state()to ensure the skills are reloaded from scratch.
| const unsub = globalSDK.event.listen((e) => { | ||
| const directory = e.name | ||
| const event = e.details | ||
| const event = e.details as any |
There was a problem hiding this comment.
Using as any for e.details bypasses TypeScript's type safety and can lead to runtime errors if the event structure changes. It's better to cast to unknown and use type guards, or at least provide a more specific partial type to ensure some level of safety.
const event = e.details as { type: string; [key: string]: unknown };
|
|
||
| sdk.event.listen((e) => { | ||
| const event = e.details | ||
| const event = e.details as any |
There was a problem hiding this comment.
The use of as any for e.details compromises type safety. This can hide bugs and make the code harder to maintain. Please consider using a more specific type, or casting to unknown and using type guards to safely access event properties.
const event = e.details as { type: string; properties: any };
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6de5f81508
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| try { | ||
| // Invalidate and reload state | ||
| const result = await state() | ||
| Bus.publish(Events.Updated, result.config) |
There was a problem hiding this comment.
The hot-reload handler calls state() directly, but Config.state is memoized via Instance.state, so this just returns the cached config instead of re-reading from disk; as a result, edits to opencode.json (and any dependent changes like new agents/commands or tool dirs) won’t actually be reflected even though config.updated is emitted. You likely need to invalidate the cached state (e.g., via Instance.invalidate(...)) before calling state() here.
Useful? React with 👍 / 👎.
packages/opencode/src/skill/skill.ts
Outdated
| try { | ||
| // Reload skills and emit update event | ||
| await state() | ||
| Bus.publish(Events.Updated, {}) |
There was a problem hiding this comment.
The skill watcher also calls state() without invalidating the Instance.state cache, so skill additions/edits/deletions keep returning the old skill list and the “skill.updated” event fires with stale data. Any change to SKILL.md will therefore not show up in Skill.all() unless the instance is restarted.
Useful? React with 👍 / 👎.
packages/opencode/src/bus/index.ts
Outdated
| await Promise.all(pending) | ||
| GlobalBus.emit("event", { | ||
| directory: Instance.directory, | ||
| payload, |
There was a problem hiding this comment.
Because Bus.publish now awaits all subscriber promises before emitting to GlobalBus, any subscriber rejection (e.g., ShareNext sync failing on a network error) will prevent the global event from ever being emitted; this means the TUI/server event streams can silently stop updating when a single subscriber throws. Consider isolating subscriber failures or emitting to GlobalBus regardless of subscriber success.
Useful? React with 👍 / 👎.
Summary
Changes
Tests
config-delete-hot-reload.test.ts- tests config deletion and reload behaviorskill-hot-reload.test.ts- comprehensive skill hot-reload scenariosFiles changed
packages/opencode,packages/app