fix(opencode): address logger correctness and prompt dedup review feedback#126
fix(opencode): address logger correctness and prompt dedup review feedback#126
Conversation
…log promises, reset client per-invocation, fix system.transform dedup, use changeset for versioning Agent-Logs-Url: https://github.com/Looted/kibi/sessions/57278c42-7fda-4283-b6b6-1cf55c043aa9 Co-authored-by: Looted <6255880+Looted@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Fixes correctness issues in the kibi-opencode plugin around structured logging and system.transform prompt injection deduplication, and aligns release metadata with Changesets-managed versioning.
Changes:
- Exported
PluginClientand made logger calls fire-and-forget with rejection handling. - Reset logger client on plugin initialization and fixed
system.transforminjection to append only the guidance block (no duplication). - Reverted manual version/changelog edits and added a Changeset for the patch release.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/opencode/src/logger.ts | Exports PluginClient; adds .catch(console.error) to avoid unhandled promise rejections in logging. |
| packages/opencode/src/index.ts | Resets logger client per invocation; updates system.transform injection to avoid duplicating prior system entries. |
| packages/opencode/package.json | Reverts version to 0.5.2 so Changesets controls the next bump. |
| packages/opencode/CHANGELOG.md | Removes the manually-added 0.5.3 entry. |
| .changeset/opencode-plugin-logging-remediation.md | Adds a Changeset describing the patch-level logging/prompt fixes. |
Comments suppressed due to low confidence (2)
packages/opencode/src/logger.ts:36
- The comment says there is a fallback when no client is available, but
info()now becomes a no-op in that case. Update the comment to reflect the current behavior (silenced logging) or implement an actual fallback to avoid misleading future maintainers.
// Fallback when no client is available (e.g. during tests or early init)
}
packages/opencode/src/logger.ts:53
- The comment says there is a fallback when no client is available, but
warn()now becomes a no-op in that case. Update the comment to match the current behavior (silenced logging) or add a real fallback implementation.
// Fallback when no client is available
}
| // Reset the logger client first to avoid leaking a previous invocation's | ||
| // client into this instance, then set the new one if provided. | ||
| logger.resetClient(); | ||
| if (input.client) { | ||
| logger.setClient(input.client); |
There was a problem hiding this comment.
logger.resetClient() is called after the early return when cfg.enabled is false, so a previously set module-level logger client can still be used (and remain set) during a disabled invocation. To fully prevent client leakage across invocations, reset the logger client before any returns (including the disabled branch) and before emitting the "disabled" log message.
| // Skip if sentinel already present in any existing entry | ||
| if (output.system.some((entry: string) => entry.includes(SENTINEL))) { | ||
| return; | ||
| } | ||
| // Build only the guidance block and append it; existing entries are preserved | ||
| const guidance = buildPrompt({ | ||
| recentEdits, | ||
| workspaceHealth, | ||
| hasRecentKbEdit, | ||
| recentCommentSuggestion, | ||
| }); | ||
| // Append-only: preserve existing system entries | ||
| if (injected === currentSystem) { | ||
| return; | ||
| } | ||
| const last = | ||
| output.system.length > 0 | ||
| ? output.system[output.system.length - 1] | ||
| : undefined; | ||
| if (last !== injected) { | ||
| output.system.push(injected); | ||
| if (last !== guidance) { | ||
| output.system.push(guidance); |
There was a problem hiding this comment.
There’s no test asserting that the system.transform hook appends only the guidance block (and does not duplicate existing output.system entries by concatenating them into the appended string). The existing hook-contract test would pass even with the previous duplication bug. Add a regression test that verifies the appended entry does not contain prior system entries and that a second transform call does not append again once the sentinel is present.
Several correctness issues were identified in the
kibi-opencodeplugin's logger and prompt injection logic. This addresses all review comments from the PR thread.Logger (
logger.ts)PluginClient— was private, causingtscdeclaration emit failure withdeclaration: trueclient.app.log()Promises were unhandled; now usevoid ... .catch(console.error)ininfo,warn, anderrorPlugin lifecycle (
index.ts)Reset client per invocation —
logger.resetClient()is now called unconditionally at the start of each plugin call beforesetClient, preventing a previous client from leaking into later instantiationssystem.transformdeduplication fix — previously calledinjectPrompt(output.system.join("\n"), ...)and pushed the full result, duplicating all prior entries. Now checks for the sentinel across existing entries and pushes onlybuildPrompt(context):Release metadata
package.jsonversion bump (0.5.3 → 0.5.2) and hand-editedCHANGELOG.mdentry.changeset/opencode-plugin-logging-remediation.md) so the Changesets workflow manages versioning💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.