fix: bridge state is now set to badcredentials when a user gets logge…#200
Conversation
…d out from another instance
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
🚧 Files skipped from review as they are similar to previous changes (6)
📜 Recent review details⏰ Context from checks skipped due to timeout. (4)
🧰 Additional context used📓 Path-based instructions (2)**/*.go📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/!(ltsm)/**/*.go📄 CodeRabbit inference engine (AGENTS.md)
Files:
🔇 Additional comments (4)
📝 WalkthroughSummary by CodeRabbit
WalkthroughUpdates LINE logout detection and session recovery, routes logged-out handling through a shared callback, adds override-based credential login, and expands tests across connector auth, reactions, ltsm crypto/embind, and runner secrets. ChangesForced logout detection and session recovery
Reaction API and connector reaction tests
LTSM crypto, embind, and runner secret tests
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![indent-zero[bot]](https://avatars.githubusercontent.com/in/1426746?s=60&v=4){kind=small}
| return false | ||
| } | ||
| return strings.Contains(err.Error(), "V3_TOKEN_CLIENT_LOGGED_OUT") | ||
| return strings.Contains(err.Error(), "V3_TOKEN_CLIENT_LOGGED_OUT") || IsInvalidSenderKey(err) |
There was a problem hiding this comment.
Code 83 may be too broad for IsLoggedOut: every API call site now reads code 83 ("invalid sender key") as a forced-logout signal, which means markLoggedOutByOtherClient blanks the access token and the bridge enters StateBadCredentials after a single such error — even when it bubbles up from message decryption / sendMessage / E2EE key resolution where it could simply mean a peer's sender key rotated. Can you confirm code 83 always implies the Chrome-extension session is gone? If not, consider scoping the IsInvalidSenderKey mapping to specific call sites (e.g., token validation only) instead of routing it through the generic IsLoggedOut.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/connector/client.go (1)
106-116: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftDon't clear
sessionInvalidatedfrom generic token writes.
setTokenscan still run in a refresh/relogin goroutine that started beforemarkLoggedOutByOtherClient(). If that older request stores a non-empty token here, it flipssessionInvalidatedback tofalseand can undo the forced-reconnect gate before the user clicks Reconnect. Reset this flag only from an explicit user-driven reconnect path, or guard token writes with an invalidation epoch so stale recoveries can't re-arm the session.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/connector/client.go` around lines 106 - 116, Generic token writes in LineClient.setTokens are incorrectly clearing sessionInvalidated, which can undo the forced-reconnect state from markLoggedOutByOtherClient. Update setTokens so it only assigns AccessToken/RefreshToken under tokenMu and does not reset sessionInvalidated for ordinary refresh/relogin flows. If needed, move the flag reset to the explicit user-driven reconnect path or add an invalidation epoch check so stale goroutines cannot re-arm the session.
🧹 Nitpick comments (1)
pkg/connector/auth_recovery_test.go (1)
203-207: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the invalidation helper instead of toggling the field directly.
Setting
lc.sessionInvalidatedhere bypasses the invalidation API added in this cohort, so this test can miss regressions if invalidation later clears more state than the boolean. Drive the setup through the helper used by production code instead.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/connector/auth_recovery_test.go` around lines 203 - 207, The test is setting the session invalidation flag directly instead of using the invalidation helper, which can bypass any additional state cleanup. Update the setup in auth_recovery_test.go to drive invalidation through the same helper used by production code on the lc instance, and keep the assertion against isTokenError(errAuthRequired) unchanged so the test exercises the real invalidation path.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/connector/connector.go`:
- Line 324: The PIN continuation path in loginWithCredentials is using
ll.Certificate, but fresh logins can leave that field empty because the
certificate only arrives in the res.Certificate branch. Update the login flow
around the res.Certificate handling and the AwaitingPIN continuation so the
challenge certificate is persisted on the login state before any PIN retry, then
have the continuation reuse that stored certificate instead of relying on
ll.Certificate being present.
In `@pkg/connector/sync.go`:
- Around line 1239-1241: The SSE auth retry path in sync logic is bypassing the
centralized token-recovery gate, so update the 401/403 handling in the sync flow
to consult shouldAttemptTokenRecovery before calling recoverToken. Keep the
behavior in the existing SSE error branch, but route recovery through the same
predicate used elsewhere so session-invalidated safeguards and any other
centralized conditions are honored. Use the lc.recoverToken and
shouldAttemptTokenRecovery symbols to locate and align this logic.
In `@pkg/ltsm/embind_test.go`:
- Around line 307-311: The roundtrip test is ignoring errors from
imp.ReadEmvalBytes, which can hide a failed export and make the equality check
pass on empty data. Update the affected roundtrip assertions to capture and
check the ReadEmvalBytes error right after exporting the key, and apply the same
fix in the matching re-import path so both paths fail fast instead of comparing
potentially empty byte slices.
In `@pkg/ltsm/vector_test.go`:
- Line 10: The vector test helpers are currently always emitting sensitive
material and writing to fixed temp files, so gate that behavior behind an
explicit opt-in in vector_test.go. Update the export paths in the relevant test
helpers and calls so dumps only happen when enabled, and ensure any file
creation uses restrictive 0600 permissions; use the existing test/vector export
symbols in the file to locate the dump logic and adjust the shared helper paths
consistently.
In `@pkg/ltsm/wbc_test.go`:
- Around line 56-69: Before slicing mod.mem with the hard-coded addresses in the
WBC test, add an explicit memory bounds check so the test fails clearly instead
of panicking if NewModule changes the allocation size. Update the setup around
xAddr, yAddr, dstAddr in the test to validate that each address range plus the
66-byte inputs fits within mod.mem before calling copy, and use a test failure
message that points to the offending address range. Also apply the same guard to
the other hard-coded memory write at the second referenced location in the test.
- Around line 83-89: The Mix66 test currently logs a success message even after
a mismatch because the loop in wbc_test.go only calls t.Errorf and then
continues. Update the test logic around the transOut/refOut comparison so that
the success log after the loop is only reached when no mismatches are found,
using the existing comparison loop in the test function (or a helper around it)
to short-circuit or track failures before calling t.Logf.
In `@pkg/runner_vectors_replay_test.go`:
- Around line 7-13: Update the skip note in TestReplayLTSMVectors so the
replacement-test breadcrumb points to the current LTSM coverage under
pkg/ltsm/*.go instead of pkg/wasm/ltsm_test.go. Keep the comment accurate to the
wazero-based Runner rewrite and ensure the reference name stays tied to
TestReplayLTSMVectors so future maintainers can find the right test location.
---
Outside diff comments:
In `@pkg/connector/client.go`:
- Around line 106-116: Generic token writes in LineClient.setTokens are
incorrectly clearing sessionInvalidated, which can undo the forced-reconnect
state from markLoggedOutByOtherClient. Update setTokens so it only assigns
AccessToken/RefreshToken under tokenMu and does not reset sessionInvalidated for
ordinary refresh/relogin flows. If needed, move the flag reset to the explicit
user-driven reconnect path or add an invalidation epoch check so stale
goroutines cannot re-arm the session.
---
Nitpick comments:
In `@pkg/connector/auth_recovery_test.go`:
- Around line 203-207: The test is setting the session invalidation flag
directly instead of using the invalidation helper, which can bypass any
additional state cleanup. Update the setup in auth_recovery_test.go to drive
invalidation through the same helper used by production code on the lc instance,
and keep the assertion against isTokenError(errAuthRequired) unchanged so the
test exercises the real invalidation path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e90093c9-cfeb-4c98-a43e-e2c8695884cb
📒 Files selected for processing (23)
pkg/connector/auth_recovery.gopkg/connector/auth_recovery_test.gopkg/connector/client.gopkg/connector/connector.gopkg/connector/creategroup.gopkg/connector/e2ee_keys.gopkg/connector/forced_logout_test.gopkg/connector/handle_message.gopkg/connector/handlers/handler.gopkg/connector/reaction_test.gopkg/connector/sync.gopkg/connector/userinfo.gopkg/line/errors.gopkg/line/errors_test.gopkg/line/reaction_test.gopkg/ltsm/crypto_test.gopkg/ltsm/embind_debug_test.gopkg/ltsm/embind_test.gopkg/ltsm/mix_test.gopkg/ltsm/vector_test.gopkg/ltsm/wbc_test.gopkg/runner_secret_test.gopkg/runner_vectors_replay_test.go
📜 Review details
⏰ Context from checks skipped due to timeout. (3)
- GitHub Check: build-docker
- GitHub Check: Lint with 1.25
- GitHub Check: Lint with 1.25
🧰 Additional context used
📓 Path-based instructions (3)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Usego fmtfor code formatting across all Go files
Usegoimportswith-local "github.com/highesttt/matrix-line-messenger"flag to group project-local imports correctly
Usezerologfor logging throughout the codebase
Do not useMsgfin logging; useMsgwith structured fields instead
UseStringerinterface where applicable in Go code
Files:
pkg/connector/handlers/handler.gopkg/ltsm/wbc_test.gopkg/ltsm/mix_test.gopkg/line/errors_test.gopkg/runner_vectors_replay_test.gopkg/ltsm/embind_debug_test.gopkg/connector/handle_message.gopkg/ltsm/vector_test.gopkg/connector/creategroup.gopkg/line/errors.gopkg/connector/reaction_test.gopkg/connector/forced_logout_test.gopkg/connector/e2ee_keys.gopkg/runner_secret_test.gopkg/connector/auth_recovery_test.gopkg/ltsm/crypto_test.gopkg/line/reaction_test.gopkg/connector/auth_recovery.gopkg/connector/userinfo.gopkg/connector/sync.gopkg/ltsm/embind_test.gopkg/connector/connector.gopkg/connector/client.go
**/!(ltsm)/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/!(ltsm)/**/*.go: Runstaticcheckon all Go files excludingpkg/ltsmpackage (transpiled WASM code)
Rungo veton all Go files excludingpkg/ltsmpackage (transpiled WASM code)
Files:
pkg/connector/handlers/handler.gopkg/ltsm/wbc_test.gopkg/ltsm/mix_test.gopkg/line/errors_test.gopkg/runner_vectors_replay_test.gopkg/ltsm/embind_debug_test.gopkg/connector/handle_message.gopkg/ltsm/vector_test.gopkg/connector/creategroup.gopkg/line/errors.gopkg/connector/reaction_test.gopkg/connector/forced_logout_test.gopkg/connector/e2ee_keys.gopkg/runner_secret_test.gopkg/connector/auth_recovery_test.gopkg/ltsm/crypto_test.gopkg/line/reaction_test.gopkg/connector/auth_recovery.gopkg/connector/userinfo.gopkg/connector/sync.gopkg/ltsm/embind_test.gopkg/connector/connector.gopkg/connector/client.go
pkg/connector/connector.go
📄 CodeRabbit inference engine (AGENTS.md)
Implement
bridgev2.NetworkConnectorandbridgev2.NetworkAPIinterfaces in the connector package for bridge logic
Files:
pkg/connector/connector.go
🔇 Additional comments (22)
pkg/connector/auth_recovery_test.go (1)
17-18: LGTM!Also applies to: 192-202, 208-214
pkg/connector/forced_logout_test.go (2)
13-46: LGTM!Also applies to: 48-85
3-11: 📐 Maintainability & Code QualityRun the Go hygiene checks for
pkg/connectorbefore merge. Keep the new tests formatted withgofmt/goimports, and make surego vetandstaticcheckpass on the package.pkg/line/reaction_test.go (1)
1-141: LGTM!pkg/connector/reaction_test.go (1)
1-411: LGTM!pkg/connector/connector.go (1)
149-149: LGTM!Also applies to: 162-162, 186-210, 217-227, 358-358, 410-434, 446-446
pkg/connector/sync.go (1)
37-46: LGTM!Also applies to: 77-85, 293-312, 457-466, 500-541, 867-878, 1153-1165, 1727-1736, 1789-1797, 1872-1880, 1920-1928
pkg/connector/userinfo.go (1)
141-152: LGTM!Also applies to: 210-250, 333-341, 379-386
pkg/connector/creategroup.go (1)
34-40: LGTM!Also applies to: 162-190, 259-265
pkg/connector/e2ee_keys.go (1)
32-51: LGTM!Also applies to: 169-179
pkg/runner_secret_test.go (2)
11-124: LGTM!
126-159: 🩺 Stability & AvailabilityConfirm runner reset between env-gated tests. If
GetRunnercaches process-wide state,t.Setenv("LTSM_ENABLE_DEBUG_EXPORT", "1")won’t affect an already-initialized runner and these tests become order-dependent.pkg/line/errors.go (1)
31-42: LGTM!pkg/line/errors_test.go (1)
54-56: LGTM!pkg/connector/client.go (1)
21-31: LGTM!Also applies to: 49-52, 119-137, 219-220, 252-288, 347-350, 364-367, 458-458, 486-497, 537-543
pkg/connector/auth_recovery.go (1)
40-45: LGTM!Also applies to: 62-64, 73-82
pkg/connector/handlers/handler.go (1)
21-21: LGTM!Also applies to: 36-42
pkg/connector/handle_message.go (1)
33-33: LGTM!pkg/ltsm/crypto_test.go (1)
1-196: LGTM!pkg/ltsm/mix_test.go (1)
1-20: LGTM!pkg/ltsm/embind_debug_test.go (1)
1-54: LGTM!pkg/ltsm/wbc_test.go (1)
10-50: 📐 Maintainability & Code QualityRun gofmt on the stub import declarations.
| if ll.AwaitingPIN { | ||
| client := line.NewClient("") | ||
| res, err := client.Login(ll.Email, ll.Password, "") | ||
| res, err := loginWithCredentials(ll.Email, ll.Password, ll.Certificate) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Persist the challenge certificate before the PIN continuation.
Line 324 now depends on ll.Certificate, but the res.Certificate branch only sets AwaitingPIN; fresh logins clear ll.Certificate, so the continuation can retry with an empty certificate.
Proposed fix
if res.Certificate != "" {
+ ll.Certificate = res.Certificate
ll.AwaitingPIN = true
return &bridgev2.LoginStep{📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| res, err := loginWithCredentials(ll.Email, ll.Password, ll.Certificate) | |
| if res.Certificate != "" { | |
| ll.Certificate = res.Certificate | |
| ll.AwaitingPIN = true | |
| return &bridgev2.LoginStep{ |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/connector/connector.go` at line 324, The PIN continuation path in
loginWithCredentials is using ll.Certificate, but fresh logins can leave that
field empty because the certificate only arrives in the res.Certificate branch.
Update the login flow around the res.Certificate handling and the AwaitingPIN
continuation so the challenge certificate is persisted on the login state before
any PIN retry, then have the continuation reuse that stored certificate instead
of relying on ll.Certificate being present.
| h1, err := imp.CallMethod("SecureKey", "exportKey", skPtr) | ||
| if err != nil { | ||
| t.Fatalf("exportKey failed: %v", err) | ||
| } | ||
| key1, _ := imp.ReadEmvalBytes(h1) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Don't ignore embind read errors in the roundtrip assertion.
Ignoring ReadEmvalBytes on Line 311 and Line 339 can turn this into a false positive: the re-import path may be fed empty bytes, and both signatures can collapse to "" while the equality check still passes.
Suggested fix
h1, err := imp.CallMethod("SecureKey", "exportKey", skPtr)
if err != nil {
t.Fatalf("exportKey failed: %v", err)
}
- key1, _ := imp.ReadEmvalBytes(h1)
+ key1, err := imp.ReadEmvalBytes(h1)
+ if err != nil {
+ t.Fatalf("read exportKey result failed: %v", err)
+ }
imp.emval.DecRef(h1)
+ if len(key1) == 0 {
+ t.Fatal("exportKey returned empty bytes")
+ }
t.Logf("exported key: %d bytes", len(key1))
@@
- sig, _ := imp.ReadEmvalBytes(rh)
+ sig, err := imp.ReadEmvalBytes(rh)
+ if err != nil {
+ t.Fatalf("read digest result failed: %v", err)
+ }
imp.emval.DecRef(rh)
return base64.StdEncoding.EncodeToString(sig)
}Also applies to: 326-341
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/ltsm/embind_test.go` around lines 307 - 311, The roundtrip test is
ignoring errors from imp.ReadEmvalBytes, which can hide a failed export and make
the equality check pass on empty data. Update the affected roundtrip assertions
to capture and check the ReadEmvalBytes error right after exporting the key, and
apply the same fix in the matching re-import path so both paths fail fast
instead of comparing potentially empty byte slices.
| "encoding/binary" | ||
| "encoding/hex" | ||
| "encoding/json" | ||
| "fmt" |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Gate vector dumps instead of emitting them on every test run.
These tests currently dump private-key/shared-secret vector material to stdout/logs and fixed world-readable /tmp files. Make vector export opt-in and write with 0600 permissions.
Proposed adjustment
- "fmt"
"os"
"testing"
@@
- t.Logf("\n--- JSON Test Vector ---\n%s", string(jsonBytes))
+ if os.Getenv("LTSM_LOG_TEST_VECTORS") != "" {
+ t.Logf("\n--- JSON Test Vector ---\n%s", string(jsonBytes))
+ }
- // Write to file
- if err := os.WriteFile("/tmp/ltsm_v2_test_vector.json", jsonBytes, 0644); err != nil {
- t.Logf("Warning: could not write test vector file: %v", err)
- } else {
- t.Logf("Test vector written to /tmp/ltsm_v2_test_vector.json")
+ if out := os.Getenv("LTSM_V2_TEST_VECTOR_PATH"); out != "" {
+ if err := os.WriteFile(out, jsonBytes, 0600); err != nil {
+ t.Logf("Warning: could not write test vector file: %v", err)
+ } else {
+ t.Logf("Test vector written to %s", out)
+ }
}
@@
- fmt.Println(string(jsonBytes))
+ if os.Getenv("LTSM_LOG_TEST_VECTORS") != "" {
+ t.Logf("%s", string(jsonBytes))
+ }
- // Write to file
- if err := os.WriteFile("/tmp/ltsm_v2_deterministic_vector.json", jsonBytes, 0644); err != nil {
- t.Logf("Warning: could not write test vector file: %v", err)
- } else {
- t.Logf("Test vector written to /tmp/ltsm_v2_deterministic_vector.json")
+ if out := os.Getenv("LTSM_V2_DETERMINISTIC_VECTOR_PATH"); out != "" {
+ if err := os.WriteFile(out, jsonBytes, 0600); err != nil {
+ t.Logf("Warning: could not write test vector file: %v", err)
+ } else {
+ t.Logf("Test vector written to %s", out)
+ }
}Also applies to: 197-204, 369-377
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/ltsm/vector_test.go` at line 10, The vector test helpers are currently
always emitting sensitive material and writing to fixed temp files, so gate that
behavior behind an explicit opt-in in vector_test.go. Update the export paths in
the relevant test helpers and calls so dumps only happen when enabled, and
ensure any file creation uses restrictive 0600 permissions; use the existing
test/vector export symbols in the file to locate the dump logic and adjust the
shared helper paths consistently.
| xAddr := uint32(8000000) | ||
| yAddr := uint32(8001000) | ||
| dstAddr := uint32(8002000) | ||
|
|
||
| // Create test inputs (base-8 digits, 0..7) | ||
| var x, y [66]byte | ||
| for i := 0; i < 66; i++ { | ||
| x[i] = byte((i * 3) % 8) | ||
| y[i] = byte((i * 5) % 8) | ||
| } | ||
|
|
||
| // Copy inputs to module memory | ||
| copy(mod.mem[xAddr:], x[:]) | ||
| copy(mod.mem[yAddr:], y[:]) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Validate module memory before slicing hard-coded offsets.
If NewModule changes its memory size, these slices panic instead of producing a useful test failure.
Proposed guard
xAddr := uint32(8000000)
yAddr := uint32(8001000)
dstAddr := uint32(8002000)
+ requiredMem := int(dstAddr) + 66
+ if len(mod.mem) < requiredMem {
+ t.Fatalf("module memory too small: len=%d, need=%d", len(mod.mem), requiredMem)
+ }Also applies to: 80-80
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/ltsm/wbc_test.go` around lines 56 - 69, Before slicing mod.mem with the
hard-coded addresses in the WBC test, add an explicit memory bounds check so the
test fails clearly instead of panicking if NewModule changes the allocation
size. Update the setup around xAddr, yAddr, dstAddr in the test to validate that
each address range plus the 66-byte inputs fits within mod.mem before calling
copy, and use a test failure message that points to the offending address range.
Also apply the same guard to the other hard-coded memory write at the second
referenced location in the test.
| for i := 0; i < 66; i++ { | ||
| if transOut[i] != refOut[i] { | ||
| t.Errorf("mismatch at index %d: transpiled=%d, reference=%d", i, transOut[i], refOut[i]) | ||
| } | ||
| } | ||
|
|
||
| t.Logf("transpiled mix output matches hand-written Mix66: %v", transOut[:10]) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Stop before logging success on mismatches.
t.Errorf marks the test failed but continues, so Line 89 can still log that outputs match.
Proposed fix
- for i := 0; i < 66; i++ {
+ for i := range transOut {
if transOut[i] != refOut[i] {
- t.Errorf("mismatch at index %d: transpiled=%d, reference=%d", i, transOut[i], refOut[i])
+ t.Fatalf("mismatch at index %d: transpiled=%d, reference=%d", i, transOut[i], refOut[i])
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for i := 0; i < 66; i++ { | |
| if transOut[i] != refOut[i] { | |
| t.Errorf("mismatch at index %d: transpiled=%d, reference=%d", i, transOut[i], refOut[i]) | |
| } | |
| } | |
| t.Logf("transpiled mix output matches hand-written Mix66: %v", transOut[:10]) | |
| for i := range transOut { | |
| if transOut[i] != refOut[i] { | |
| t.Fatalf("mismatch at index %d: transpiled=%d, reference=%d", i, transOut[i], refOut[i]) | |
| } | |
| } | |
| t.Logf("transpiled mix output matches hand-written Mix66: %v", transOut[:10]) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/ltsm/wbc_test.go` around lines 83 - 89, The Mix66 test currently logs a
success message even after a mismatch because the loop in wbc_test.go only calls
t.Errorf and then continues. Update the test logic around the transOut/refOut
comparison so that the success log after the loop is only reached when no
mismatches are found, using the existing comparison loop in the test function
(or a helper around it) to short-circuit or track failures before calling
t.Logf.
| // TestReplayLTSMVectors previously replayed captured JSON IPC vectors through | ||
| // the Node.js runner's call() method. Since the runner now uses wazero directly | ||
| // (no JSON IPC), this test needs a rewrite to use the typed Runner methods. | ||
| // The WASM-level correctness is covered by pkg/wasm/ltsm_test.go (sign vectors, | ||
| // AES roundtrip, Curve25519 roundtrip). | ||
| func TestReplayLTSMVectors(t *testing.T) { | ||
| t.Skip("vector replay test needs rewrite for wazero-based runner (no JSON IPC)") |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Update the replacement-test reference.
The skip note points readers to pkg/wasm/ltsm_test.go, but the supplied PR context places the current LTSM coverage under pkg/ltsm/*.go. Please update the breadcrumb to the actual replacement tests so this does not send the next maintainer to a stale path.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/runner_vectors_replay_test.go` around lines 7 - 13, Update the skip note
in TestReplayLTSMVectors so the replacement-test breadcrumb points to the
current LTSM coverage under pkg/ltsm/*.go instead of pkg/wasm/ltsm_test.go. Keep
the comment accurate to the wazero-based Runner rewrite and ensure the reference
name stays tied to TestReplayLTSMVectors so future maintainers can find the
right test location.
1 participant
…d out from another instance