Conversation
There was a problem hiding this comment.
I'm Bonk, and I've done a quick review of your PR.
This introduces a Rust worker::Interface trait mirroring workerd::WorkerInterface, with FFI bridging and two simple implementations (error worker, kill-switch worker).
Findings (highest severity first):
- [MEDIUM]
SCRIPT_KILLED_DETAIL_IDinbridge.husesconst uint64_tinstead ofconstexpr kj::Exception::DetailTypeId, inconsistent with the rest of the codebase. - [LOW]
error.rs:86usesstd::result::Result<AlarmResult, KjError>while all other methods in the same file usecrate::Result<T>— inconsistent. - [LOW] Commented-out internal references leaked from internal repo (
edgeworkernamespace in test, commented-out#includeand BUILD dep). - [LOW] Typo in
lib.rsdoc comment: "rusts's" should be "Rust's".
These comments were generated by an AI code review assistant and may contain inaccuracies.
|
Review posted successfully on PR #6621 with 5 inline comments covering:
|
6a15a48 to
ea13699
Compare
ea13699 to
114b9d1
Compare
There was a problem hiding this comment.
I'm Bonk, and I've done a quick review of your PR.
This PR introduces a Rust worker::Interface trait mirroring workerd::WorkerInterface, with FFI bridging via CXX, and two simple implementations (error worker, kill-switch worker).
Findings (highest severity first):
- [HIGH]
CPU_LIMIT_DETAIL_IDvalue mismatch between Rust and C++ — the Rust constant has a missing hex digit, causing a silently wrong value. - [LOW]
compile_flags.txtreferenceserrors.rs@cxxbut the source file iserror.rs(no trailing 's'). - [LOW] All new files (
.rs,.h,.c++) are missing copyright/license headers, which the codebase requires.
Note: the SCRIPT_KILLED_DETAIL_ID const/constexpr issue from the previous review is still valid and unaddressed.
These comments were generated by an AI code review assistant and may contain inaccuracies.
|
Review posted successfully on PR #6621 with 3 inline comments covering:
|
114b9d1 to
1ecc930
Compare
|
UnknownError: ProviderInitError |
|
@mikea Bonk workflow failed. Check the logs for details. View workflow run · To retry, trigger Bonk again. |
|
UnknownError: ProviderInitError |
1ecc930 to
8f12f3c
Compare
|
LGTM |
Merging this PR will not alter performance
Comparing Footnotes
|
17d219e to
af2a35f
Compare
jasnell
left a comment
There was a problem hiding this comment.
Couple of nits, otherwise LGTM
0090dae to
72b2473
Compare
upstreaming internal work to start building on it.
72b2473 to
861eb04
Compare
upstreaming internal work to start building on it.