fix(plugin): only call function return values when tearing down a plugin by holny · Pull Request #57775 · anomalyco/opencode · GitHub
Skip to content

fix(plugin): only call function return values when tearing down a plugin - #57775

Open
holny wants to merge 1 commit into
anomalyco:devfrom
holny:fix-57719-plugin-teardown
Open

holny wants to merge 1 commit into
anomalyco:devfrom
holny:fix-57719-plugin-teardown

Conversation

@holny

@holny holny commented Oct 21, 2026 •

Copy link
Copy Markdown
Contributor

Issue for this PR

Closes #57719

Type of change

  • Bug fix

What does this PR do?

A plugin's setup() runs as JavaScript, so it may return anything. The teardown that runs when the previous plugin generation is released only checked the returned value for truthiness and then called it:

(cleanup) => (cleanup ? Effect.promise(() => Promise.resolve(cleanup())) : Effect.void)

A plugin whose setup() returns an object therefore hits cleanup() and the reload dies with TypeError: o is not a function, which surfaces as opencode reload failing against the background service.

Only a real function counts as a cleanup callback now, so a plugin that returns something else is simply left without a teardown hook.

This is a different layer from #57096: that PR isolates a cleanup that throws so one plugin cannot abort the whole activation, while this one avoids calling a value that was never a cleanup function in the first place. The two are complementary — a plugin can still return a well-formed cleanup that throws, and #57096 covers that.

How did you verify your code works?

  • New regression test in packages/core/test/plugin/supervisor-reload.test.ts loads a local plugin whose setup() returns an object, then triggers a reload and asserts the new generation replaces the old one instead of the reload dying. Red/green checked: the previous truthiness check fails that case, the fix passes it.
  • bun test test/plugin/supervisor-reload.test.ts is 9/9.
  • bun test test/plugin/ is 396/397, with the single failure being a pre-existing timeout in module.test.ts that reproduces on the base commit.
  • bun typecheck clean in packages/core and packages/plugin.

Screenshots / recordings

N/A, not a UI change.

Checklist

  • I have tested my changes locally
  • I have not included unrelated changes in this PR

Added by the opencode bot (AI-generated).

Related: #57089

A plugin's setup() is JavaScript at runtime, so it may return anything. The
teardown that runs when the previous plugin generation is released checked
the returned value for truthiness and then called it, so a plugin whose
setup() returned an object aborted the whole reload with
"TypeError: cleanup is not a function".

Only a real function counts as a cleanup callback now.

Regression test in test/plugin/supervisor-reload.test.ts drives a reload
against a plugin whose setup() returns an object and asserts the new
generation replaces the old one. Red/green checked: the previous
truthiness check fails that case, the fix passes it.

Verification: `bun test test/plugin/supervisor-reload.test.ts` is 9/9;
`bun test test/plugin/` is 396/397 with the single failure being a
pre-existing timeout in module.test.ts that reproduces on the base
commit; `bun typecheck` clean in packages/core and packages/plugin.
@github-actions

Copy link
Copy Markdown
Contributor

@opencode-agent opencode-agent Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fix works. I ran the #57719 reproduction from source on both checkouts. On the base, with a plugin whose setup() returns an object, opencode reload fails with TypeError: cleanup is not a function (adapter.ts:621). With this PR every reload prints Configuration reloaded. The new test in supervisor-reload.test.ts fails on the base and passes here. The rest of that file shows one failure, which also fails on the base. Type checks are clean in packages/plugin and packages/core.

Public API: unchanged. A non-function value returned from setup() is now ignored instead of called as a cleanup function, so existing plugins are unaffected.

No concerns. The fix is in the right place (the adapter, where the cleanup is called) and complements #57096, which handles a cleanup that throws.

Related: #57089

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

1 participant