Skip to content

rewrite opencode plugin if content differs#775

Open
Soph wants to merge 1 commit intomainfrom
soph/do-content-check-for-opencode-plugin
Open

rewrite opencode plugin if content differs#775
Soph wants to merge 1 commit intomainfrom
soph/do-content-check-for-opencode-plugin

Conversation

@Soph
Copy link
Collaborator

@Soph Soph commented Mar 25, 2026

Entire-Checkpoint: f92b8ef4844d


Note

Low Risk
Low risk behavioral tweak limited to OpenCode hook installation; main change is that InstallHooks will now overwrite stale plugin content instead of treating any file with the marker as already installed.

Overview
Makes opencode hook installation truly idempotent by comparing the on-disk .opencode/plugins/entire.ts contents to the newly generated template output.

InstallHooks now returns no-op only when the file is exactly up-to-date (unless force), and will rewrite the plugin when content differs (e.g., localDev vs released command prefix).

Written by Cursor Bugbot for commit 13238aa. Configure here.

Entire-Checkpoint: f92b8ef4844d
@Soph Soph requested a review from a team as a code owner March 25, 2026 17:01
Copilot AI review requested due to automatic review settings March 25, 2026 17:01
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR updates the OpenCode agent hook installer to rewrite the .opencode/plugins/entire.ts plugin when the generated content has changed, instead of only treating the file as “installed” based on an embedded marker.

Changes:

  • Replace marker-based idempotency with exact content equality (skip rewrite only when identical).
  • Ensure mode/template changes (e.g., localDev vs prod command prefix) trigger a rewrite unless --force is used to override the idempotency check.

Comment on lines +61 to +67
// Check if already installed with identical content (idempotent) unless force
if !force {
if existing, readErr := os.ReadFile(pluginPath); readErr == nil { //nolint:gosec // Path constructed from repo root
if string(existing) == content {
return 0, nil // Already up-to-date
}
}
Copy link

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

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

New behavior: when the plugin file exists but its content differs (e.g., switching localDev/prod or template updates), InstallHooks now rewrites it. There isn’t a test covering this update path (only fresh/idempotent/force/localDev). Add a unit test that pre-writes a different entire.ts (or installs once with localDev=true then installs with localDev=false) and asserts InstallHooks returns 1 and rewrites the file when force=false.

Copilot generated this review using guidance from repository custom instructions.
Comment on lines +64 to +66
if string(existing) == content {
return 0, nil // Already up-to-date
}
Copy link

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

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

The InstallHooks doc comment says it returns 0 when the plugin is "already present". With the new content-equality idempotency check, a file can be present but still be rewritten (returning 1). Consider updating the wording to "0 if already up-to-date" (and/or mention that differing content will be rewritten unless --force is false).

Copilot uses AI. Check for mistakes.
// Check if already installed with identical content (idempotent) unless force
if !force {
if existing, readErr := os.ReadFile(pluginPath); readErr == nil { //nolint:gosec // Path constructed from repo root
if string(existing) == content {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm assuming this is an optimization to avoid having to overwrite the same file over and over again, correct?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As it was before we did not update it at all but just checked if it's there so I went with this. But I guess on the other side since this happens only in the context of entire enable we could also just always write it? 🤔

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

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants