Conversation
There was a problem hiding this comment.
This PR adds OpenCode GitHub Action workflows for automated PR review and comment-triggered agent interaction, alongside a reviewer prompt file.
Issues found (ranked by severity):
-
Medium --
/octrigger is an overly broad substring match. GitHub'scontains()is a case-insensitive substring match, so any comment containing/ocanywhere -- e.g./docs,/october,block/occurs-- will trigger the workflow. The existing Bonk workflow avoids this by using amentionsinput parameter on the action itself (which presumably does more precise matching). Either use a longer unique prefix or usestartsWith()+ word boundary logic. -
Low -- Both the Bonk and OpenCode review workflows will fire on the same PR events.
new-pr-review.ymltriggers onopenedandopencode-review.ymltriggers onopened, synchronize, reopened, ready_for_review. Every new PR will get two AI reviews (one from Bonk, one from OpenCode). If this is intentional for a comparison/trial period, it's fine but worth noting. If not, the existingnew-pr-review.ymlshould be removed or the triggers should be made mutually exclusive.
| if: | | ||
| github.event.sender.type != 'Bot' && | ||
| (contains(github.event.comment.body, '/oc') || | ||
| contains(github.event.comment.body, '/opencode')) |
There was a problem hiding this comment.
Medium: contains(github.event.comment.body, '/oc') is a substring match -- it will fire on any comment containing /oc anywhere (e.g. /docs, /october, block/occurs). The existing Bonk workflow uses a mentions input on the action to handle this correctly. Consider using a similar mechanism, or at minimum use a less ambiguous trigger like /opencode only:
| if: | | |
| github.event.sender.type != 'Bot' && | |
| (contains(github.event.comment.body, '/oc') || | |
| contains(github.event.comment.body, '/opencode')) | |
| if: | | |
| github.event.sender.type != 'Bot' && | |
| contains(github.event.comment.body, '/opencode') |
|
Bonk workflow was cancelled. View workflow run · To retry, trigger Bonk again. |
|
I'm Bonk, and I've done a quick review of your PR. This PR adds OpenCode GitHub Action workflows (auto-review on PRs and comment-triggered agent) plus a reviewer prompt file that's nearly identical to the existing Bonk one. Issues posted as review comments:
|
|
I don't have GitHub CLI access to post review comments. I'll provide my review inline here instead. I'm OpenCode, and I've done a quick review of your PR. Summary: Adds two GitHub Actions workflows for OpenCode integration — an automated PR reviewer ( Issues (ranked by severity):
|
There was a problem hiding this comment.
Bonk Review
Findings from automated review (generated by an AI assistant -- may contain errors):
Issues
[LOW] Unused include in export-loopback.c++
#include <workerd/io/features.h> at line 7 is not used anywhere in the file. FeatureFlags and nothing else from features.h appear in this translation unit. This was also flagged by jp4a50 -- looks like it may be left over from an earlier approach.
[LOW] versionRequest parameter silently ignored in server.c++ override
In server.c++:3294-3312, the getSubrequestChannel override accepts the new versionRequest parameter but never uses it. The function only acts on props -- if props is kj::none, it simply adds a ref to the existing channel. The PR description acknowledges this ("workerd's server implementation currently ignores this information"), but it would be good to add a brief inline comment (e.g. // TODO: versionRequest is currently ignored by workerd) so future readers don't have to check the PR description to understand this is intentional.
[LOW] Test only verifies the version info doesn't break anything, not that it's propagated
The test at server-test.c++:4453 calls ctx.exports.default(...) with various version shapes and then checks that service.version returns the constant getter value. This confirms the call doesn't blow up, but since the version is silently ignored in workerd, the test doesn't verify any version-related behavior. This is noted in the PR description as intentional, which is fine for now. jp4a50 suggested also passing props and undefined in the test -- that feedback still appears unaddressed.
Observations (non-blocking)
-
The
jsg::Optional<kj::Maybe<kj::String>> cohorttype correctly models the JS semantics wherecohortcan be absent (undefined), explicitly null, or a string. This follows the established codebase pattern (e.g.actor.h:172). -
The compat flag approach (two separate
JSG_CALLABLEregistrations gated onflags.getEnableVersionApi()) cleanly preserves backward compatibility. The discussion between jasnell and sohpeach about whether to have a singlecallmethod vs. two methods is resolved -- the current approach avoids subtle type-checking changes for existing users who don't have the flag enabled. -
The
VersionRequest::clone()method is well-implemented usingcohort.map(...). -
The
OptionsWithVersion::VersionandOptionsWithVersionstructs are correctly registered inEW_EXPORT_LOOPBACK_ISOLATE_TYPES. -
Generated TypeScript snapshots look correct and match the C++ type definitions.
This review was generated by an AI assistant and may contain inaccuracies. Please verify any suggestions before applying them.
|
|
||
| ## Review focus areas | ||
|
|
||
| **C++ code quality:** Correct use of KJ patterns (kj::Own, kj::Maybe, kj::Promise, kj::Array). Check ownership semantics, lifetime issues, and proper error propagation via kj::Exception. |
There was a problem hiding this comment.
for consistency, you could have this point to the existing .opencode/skills that I've added. There are some there for kj-style, workerd-api, workerd-safety. And I just opened another PR that adds rust, js, and ts checklists.
I did primarily write those for a local workflow but they ought to work for CI as well.

No description provided.