feat(cloud): jcloud device login + relay connector + E2EE#163
Conversation
- config cloud block (enabled/url/auto_connect) with mutex snapshot accessors - ~/.jcode/cloud.json credentials (0600, atomic write) + X25519 identity keypair - internal/cloud client: device code poll, register, URL validation (https; localhost http dev) - jcode login/logout/--status subcommands
- internal/cloud connector: register/heartbeat/poll loops, command dispatch to local control plane, WS event pump with durable/ephemeral split, per-session seq with server-seeded resume, session index mirror - /api/chat accepts optional source field for channel tagging
- accumulate agent_text deltas per session, upload finalized text as durable event - buffer cap + session_reset cleanup; relay e2e J8 asserts assistant text replay
… (M5) - CEK lifecycle in cloud.json + BIP39 recovery phrase + rotate-key - connector seals uplink (events/ephemeral/meta/ack) and opens downlink, gray plaintext compat - jcode cloud command group: pairings/approve/deny/status/key show-phrase/key recover/rotate-key - cloud.e2ee config switch (default on); cross-impl test vectors verified both directions
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds jcode Cloud device login, encrypted relay connectivity, pairing and key recovery, CLI and web controls, configuration, synchronization, tests, and documentation. ChangesCloud device relay and integration
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant JCodeCLI
participant LocalWeb
participant Cloud
User->>JCodeCLI: jcode login
JCodeCLI->>Cloud: request and poll device token
JCodeCLI->>Cloud: register device
User->>LocalWeb: start jcode web
LocalWeb->>Cloud: heartbeat and poll commands
Cloud-->>LocalWeb: encrypted command
LocalWeb->>LocalWeb: decrypt and execute locally
LocalWeb->>Cloud: encrypted acknowledgement
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
internal/command/login.go (1)
105-107: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRoute warning diagnostics through
config.Logger()too.These failure warnings (browser-open failure, config-update failure, revoke failure) are printed to stdout only.
web.go's equivalent pattern (resolveWebToken/runWebServer) logs the same class of failure viaconfig.Logger()in addition to the user-facing message. As per coding guidelines,**/*.go: "Send all diagnostics throughconfig.Logger(); never usefmt.Print... or write directly to stdout/stderr." These are diagnostic warnings, not the command's primary output, and losing them once the terminal scrolls away makes post-hoc debugging harder.♻️ Example for one site (repeat for the others)
if err := client.RevokeDevice(ctx, creds.DeviceToken); err != nil { + config.Logger().Printf("[cloud] failed to revoke device token on %s: %v", creds.CloudURL, err) fmt.Printf("Warning: failed to revoke device token on %s: %v\n", creds.CloudURL, err) fmt.Println("Clearing local credentials anyway.") }Also applies to: 151-153, 176-178, 185-187
🤖 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 `@internal/command/login.go` around lines 105 - 107, Update the login command’s browser-open, configuration-update, and revoke failure paths to send diagnostic details through config.Logger() instead of relying on stdout-only fmt.Printf calls. Preserve the existing user-facing warnings while also logging each failure through the configured logger, covering the openBrowser(dc.VerificationURI) path and the corresponding config-update and revoke handling.Source: Coding guidelines
🤖 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 `@internal/cloud/crypto.go`:
- Around line 149-162: Add a nonce-length validation before each GCM decrypt
call: in internal/cloud/crypto.go lines 149-162, update Open to reject decoded
nonces whose length differs from e.aead.NonceSize() before e.aead.Open; apply
the same guard in UnwrapCEK at internal/cloud/crypto.go lines 480-491 before
aead.Open, returning an error rather than allowing malformed peer input to
panic.
In `@internal/cloud/events.go`:
- Around line 284-299: Update Connector.eventPumpLoop so bo.Reset() is not
called after every pumpEvents failure; move the reset to the successful
connection establishment path, such as immediately after the local event stream
dial succeeds. Preserve backoff growth across failed reconnect attempts while
retaining reset behavior once a live connection has been confirmed.
In `@internal/command/login.go`:
- Around line 124-149: Update the runLogin flow around RegisterDevice and
cloud.SaveCredentials so a credentials-save failure triggers best-effort cleanup
of the newly registered remote device before returning the error. Reuse the
registered device identifier and existing client/API cleanup operation, preserve
the original SaveCredentials error as the returned failure, and keep successful
registration and credential persistence unchanged.
- Around line 96-112: Validate dc.VerificationURI before passing it to
openBrowser, accepting only http and https schemes and rejecting all others.
Preserve the existing manual-URL message and continue to PollForToken with the
already-clamped interval and expiration values.
---
Nitpick comments:
In `@internal/command/login.go`:
- Around line 105-107: Update the login command’s browser-open,
configuration-update, and revoke failure paths to send diagnostic details
through config.Logger() instead of relying on stdout-only fmt.Printf calls.
Preserve the existing user-facing warnings while also logging each failure
through the configured logger, covering the openBrowser(dc.VerificationURI) path
and the corresponding config-update and revoke handling.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: bba74f24-6ed5-4216-a2ad-46f3984ef944
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (30)
cmd/jcode/main.godocs/cloud.mdgo.modinternal/cloud/backoff.gointernal/cloud/backoff_test.gointernal/cloud/client.gointernal/cloud/client_test.gointernal/cloud/connector.gointernal/cloud/connector_test.gointernal/cloud/credentials.gointernal/cloud/crypto.gointernal/cloud/crypto_test.gointernal/cloud/crypto_wiring_test.gointernal/cloud/events.gointernal/cloud/events_unit_test.gointernal/cloud/identity_test.gointernal/cloud/pairings.gointernal/cloud/pairings_test.gointernal/cloud/relay.gointernal/cloud/sessions.gointernal/command/cloud.gointernal/command/cloud_cmd_test.gointernal/command/cloud_test.gointernal/command/login.gointernal/command/login_test.gointernal/command/web.gointernal/config/cloud_settings_test.gointernal/config/config.gointernal/prompts/memory_test.gointernal/web/chat.go
| nonce, err := base64.StdEncoding.DecodeString(env.Nonce) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("open: nonce: %w", err) | ||
| } | ||
| ct, err := base64.StdEncoding.DecodeString(env.CT) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("open: ct: %w", err) | ||
| } | ||
| plain, err := e.aead.Open(nil, nonce, ct, nil) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("open: decrypt failed (wrong key or corrupted data): %w", err) | ||
| } | ||
| return plain, nil | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Validate nonce length before every GCM Open — malformed peer nonces panic instead of erroring. Both decrypt paths decode a peer-controlled base64 nonce and hand it directly to GCM Open, which panics when the length differs from NonceSize(). The root fix is a length check before each Open.
internal/cloud/crypto.go#L149-L162: inOpen, rejectnoncewhose length !=e.aead.NonceSize()before callinge.aead.Open(this path is reachable from the connector's downlink command decrypt viaOpenMaybe).internal/cloud/crypto.go#L480-L491: inUnwrapCEK, apply the sameaead.NonceSize()guard beforeaead.Open.
📍 Affects 1 file
internal/cloud/crypto.go#L149-L162(this comment)internal/cloud/crypto.go#L480-L491
🤖 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 `@internal/cloud/crypto.go` around lines 149 - 162, Add a nonce-length
validation before each GCM decrypt call: in internal/cloud/crypto.go lines
149-162, update Open to reject decoded nonces whose length differs from
e.aead.NonceSize() before e.aead.Open; apply the same guard in UnwrapCEK at
internal/cloud/crypto.go lines 480-491 before aead.Open, returning an error
rather than allowing malformed peer input to panic.
| func (c *Connector) eventPumpLoop(ctx context.Context) { | ||
| bo := c.backoff() | ||
| batcher := newEventBatcher(c) | ||
| go batcher.run(ctx) | ||
| for { | ||
| err := c.pumpEvents(ctx, batcher) | ||
| if ctx.Err() != nil { | ||
| return | ||
| } | ||
| c.logf("local event stream disconnected: %v", err) | ||
| bo.Reset() // a live connection resets the backoff before the next wait | ||
| if werr := bo.Wait(ctx); werr != nil { | ||
| return | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Unconditional bo.Reset() defeats the reconnect backoff.
bo.Reset() runs on every loop iteration before bo.Wait(ctx), so Wait always sleeps Min. When pumpEvents fails immediately (e.g. the local WS endpoint is transiently unreachable), the reconnect loop never grows the delay — it retries at the Min interval indefinitely, unlike pollLoop/registerLoop, which only reset after a confirmed-good state. The reset should happen only once a live connection is actually established (after a successful dial), not on every failed attempt.
🔧 Proposed fix: reset only after a successful dial
func (c *Connector) eventPumpLoop(ctx context.Context) {
bo := c.backoff()
batcher := newEventBatcher(c)
go batcher.run(ctx)
for {
- err := c.pumpEvents(ctx, batcher)
+ err := c.pumpEvents(ctx, batcher, bo)
if ctx.Err() != nil {
return
}
c.logf("local event stream disconnected: %v", err)
- bo.Reset() // a live connection resets the backoff before the next wait
if werr := bo.Wait(ctx); werr != nil {
return
}
}
}-func (c *Connector) pumpEvents(ctx context.Context, batcher *eventBatcher) error {
+func (c *Connector) pumpEvents(ctx context.Context, batcher *eventBatcher, bo *Backoff) error {
...
conn, _, err := dialer.DialContext(ctx, wsURL, header)
if err != nil {
return err
}
defer func() { _ = conn.Close() }()
+ bo.Reset() // a live connection resets the backoff for the next reconnect🤖 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 `@internal/cloud/events.go` around lines 284 - 299, Update
Connector.eventPumpLoop so bo.Reset() is not called after every pumpEvents
failure; move the reset to the successful connection establishment path, such as
immediately after the local event stream dial succeeds. Preserve backoff growth
across failed reconnect attempts while retaining reset behavior once a live
connection has been confirmed.
| fmt.Println() | ||
| fmt.Println("To sign in to jcloud, open this page in your browser:") | ||
| fmt.Println() | ||
| fmt.Printf(" %s\n", dc.VerificationURI) | ||
| fmt.Println() | ||
| fmt.Println("and enter this code:") | ||
| fmt.Println() | ||
| fmt.Printf(" %s\n", dc.UserCode) | ||
| fmt.Println() | ||
| if err := openBrowser(dc.VerificationURI); err != nil { | ||
| fmt.Printf("(could not open a browser automatically: %v — please open the URL manually)\n", err) | ||
| } | ||
| fmt.Println("Waiting for authorization... (Ctrl+C to cancel)") | ||
|
|
||
| interval := time.Duration(dc.Interval) * time.Second | ||
| expiresIn := time.Duration(dc.ExpiresIn) * time.Second | ||
| tok, err := client.PollForToken(ctx, dc.DeviceCode, interval, expiresIn) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Does Go's os/exec on Windows properly escape command-line arguments passed to cmd.exe /c start to prevent metacharacter interpretation?
💡 Result:
No, Go's os/exec package does not properly escape command-line arguments for cmd.exe or batch files (.bat/.cmd) to prevent metacharacter interpretation [1][2][3]. The os/exec package on Windows constructs a single command-line string from the provided arguments using an algorithm compatible with CommandLineToArgvW, which is the standard parsing rule used by most Windows applications [1][4][5]. However, cmd.exe and batch files use different, non-standard parsing and quoting rules [1][2][3]. Because of this discrepancy, os/exec's default escaping is insufficient for cmd.exe, which can lead to command injection vulnerabilities if untrusted input is passed as arguments [2][3]. The official Go documentation explicitly identifies cmd.exe, batch files, and msiexec.exe as notable exceptions to its standard escaping algorithm [1][6][5]. To safely execute these commands, the Go team recommends that developers manually format and quote the command line themselves and provide it directly via the SysProcAttr.CmdLine field, while leaving the Args field empty [1][3][5]. This issue is well-documented in the Go issue tracker, with multiple discussions regarding the risks of command injection when executing batch files or using cmd.exe /c [2][3][4][7]. There is no automatic, built-in protection in os/exec to handle the unique syntax requirements of cmd.exe [3].
Citations:
- 1: https://pkg.go.dev/os/exec?GOOS=windows
- 2: os/exec: execution of batch-files (.cmd/.bat) is vulnerable in go-lang for windows / insufficient escape golang/go#27199
- 3: syscall: special case
cmd.exe /c <command>in StartProcess golang/go#69939 - 4: os/exec: Cannot execute command with space in the name on Windows, when there are parameters golang/go#17149
- 5: https://go.dev/src/os/exec/exec.go
- 6: https://pkg.go.dev/os/exec
- 7: syscall: exec_windows.go: arguments should not be escaped to work with msiexec golang/go#15566
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== login.go ==\n'
sed -n '1,260p' internal/command/login.go
printf '\n== client.go ==\n'
sed -n '1,260p' internal/cloud/client.go
printf '\n== openBrowser usages ==\n'
rg -n "openBrowser|VerificationURI|Interval|ExpiresIn|PollForToken|SaveCredentials|RegisterDevice" internal -nRepository: cnjack/jcode
Length of output: 24221
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== PollForToken implementation ==\n'
sed -n '259,340p' internal/cloud/client.go
printf '\n== browser opener in web.go ==\n'
sed -n '720,760p' internal/web/server.go
printf '\n== login tests around device code flow ==\n'
sed -n '1,240p' internal/command/login_test.goRepository: cnjack/jcode
Length of output: 8004
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== utils.OpenURL implementation ==\n'
rg -n "func OpenURL|OpenURL\(" internal -g '*.go'Repository: cnjack/jcode
Length of output: 475
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== utils.OpenURL implementation ==\n'
rg -n "func OpenURL|OpenURL\(" internal -g '*.go'
printf '\n== utils package files ==\n'
git ls-files internal | rg '^internal/.*/utils.*\.go$|^internal/utils.*\.go$'Repository: cnjack/jcode
Length of output: 502
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,120p' internal/util/util.goRepository: cnjack/jcode
Length of output: 942
Validate the verification URI before opening it
dc.VerificationURI comes from the server and is passed straight to the platform opener. Reject non-http(s) schemes before calling openBrowser; the interval/expires-in values are already clamped in PollForToken.
🤖 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 `@internal/command/login.go` around lines 96 - 112, Validate dc.VerificationURI
before passing it to openBrowser, accepting only http and https schemes and
rejecting all others. Preserve the existing manual-URL message and continue to
PollForToken with the already-clamped interval and expiration values.
| pubKey, privKey, err := cloud.GenerateIdentityKeyPair() | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| if err := client.RegisterDevice(ctx, tok.AccessToken, cloud.RegisterDeviceRequest{ | ||
| Name: name, | ||
| Hostname: hostname, | ||
| JcodeVersion: Version, | ||
| PubKey: pubKey, | ||
| }); err != nil { | ||
| return fmt.Errorf("failed to register device: %w", err) | ||
| } | ||
|
|
||
| creds := &cloud.Credentials{ | ||
| CloudURL: baseURL, | ||
| DeviceID: tok.DeviceID, | ||
| DeviceToken: tok.AccessToken, | ||
| DeviceName: name, | ||
| PublicKey: pubKey, | ||
| PrivateKey: privKey, | ||
| KeyGen: 1, | ||
| } | ||
| if err := cloud.SaveCredentials(creds); err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Orphaned server-side device registration if SaveCredentials fails.
RegisterDevice (a non-idempotent remote write) succeeds before SaveCredentials is attempted. If the local write fails (disk full, permission issue, etc.), runLogin returns an error but the device stays registered on jcloud with no local credential to ever revoke it — a repeat jcode login will register yet another device rather than resuming/replacing the failed one.
🩹 Proposed best-effort cleanup on save failure
if err := cloud.SaveCredentials(creds); err != nil {
+ if revokeErr := client.RevokeDevice(ctx, tok.AccessToken); revokeErr != nil {
+ config.Logger().Printf("[cloud] failed to revoke orphaned device registration after save failure: %v", revokeErr)
+ }
return err
}📝 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.
| pubKey, privKey, err := cloud.GenerateIdentityKeyPair() | |
| if err != nil { | |
| return err | |
| } | |
| if err := client.RegisterDevice(ctx, tok.AccessToken, cloud.RegisterDeviceRequest{ | |
| Name: name, | |
| Hostname: hostname, | |
| JcodeVersion: Version, | |
| PubKey: pubKey, | |
| }); err != nil { | |
| return fmt.Errorf("failed to register device: %w", err) | |
| } | |
| creds := &cloud.Credentials{ | |
| CloudURL: baseURL, | |
| DeviceID: tok.DeviceID, | |
| DeviceToken: tok.AccessToken, | |
| DeviceName: name, | |
| PublicKey: pubKey, | |
| PrivateKey: privKey, | |
| KeyGen: 1, | |
| } | |
| if err := cloud.SaveCredentials(creds); err != nil { | |
| return err | |
| } | |
| pubKey, privKey, err := cloud.GenerateIdentityKeyPair() | |
| if err != nil { | |
| return err | |
| } | |
| if err := client.RegisterDevice(ctx, tok.AccessToken, cloud.RegisterDeviceRequest{ | |
| Name: name, | |
| Hostname: hostname, | |
| JcodeVersion: Version, | |
| PubKey: pubKey, | |
| }); err != nil { | |
| return fmt.Errorf("failed to register device: %w", err) | |
| } | |
| creds := &cloud.Credentials{ | |
| CloudURL: baseURL, | |
| DeviceID: tok.DeviceID, | |
| DeviceToken: tok.AccessToken, | |
| DeviceName: name, | |
| PublicKey: pubKey, | |
| PrivateKey: privKey, | |
| KeyGen: 1, | |
| } | |
| if err := cloud.SaveCredentials(creds); err != nil { | |
| if revokeErr := client.RevokeDevice(ctx, tok.AccessToken); revokeErr != nil { | |
| config.Logger().Printf("[cloud] failed to revoke orphaned device registration after save failure: %v", revokeErr) | |
| } | |
| return err | |
| } |
🤖 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 `@internal/command/login.go` around lines 124 - 149, Update the runLogin flow
around RegisterDevice and cloud.SaveCredentials so a credentials-save failure
triggers best-effort cleanup of the newly registered remote device before
returning the error. Reuse the registered device identifier and existing
client/API cleanup operation, preserve the original SaveCredentials error as the
returned failure, and keep successful registration and credential persistence
unchanged.
- connector supervisor: runtime auto_connect start/stop without restart - GET /api/cloud/status + POST /api/cloud/config; connector exposes state - sidebar CloudBadge with status popover + login guidance (5 languages) - register reports platform via JCODE_DESKTOP env (Tauri sidecar injects)
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
internal/web/cloud.go (1)
69-94: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPersist+rollback logic duplicated from
Supervisor.SetAutoConnect.This fallback re-implements, nearly verbatim, the same read/modify/write + rollback pattern already in
cloud.Supervisor.SetAutoConnect(previous := cfg.CloudSettings(); cfg.SetCloud(...); if SaveConfig fails, roll back). Extracting a shared helper (e.g.config.SetCloudAutoConnect(cfg, enabled) error) used by both sites would prevent the two copies drifting apart on a future fix.♻️ Sketch
+// in internal/config: +func SetCloudAutoConnect(cfg *Config, enabled bool) error { + previous := cfg.CloudSettings() + cfg.SetCloud(&CloudConfig{Enabled: previous.Enabled, URL: previous.URL, AutoConnect: &enabled, E2EE: previous.E2EE}) + if err := SaveConfig(cfg); err != nil { + if previous == (CloudConfig{}) { + cfg.SetCloud(nil) + } else { + cfg.SetCloud(&previous) + } + return err + } + return nil +}🤖 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 `@internal/web/cloud.go` around lines 69 - 94, Extract the shared CloudSettings read/modify/save-and-rollback behavior from Supervisor.SetAutoConnect and the nil-supervisor fallback into a helper such as config.SetCloudAutoConnect(cfg, enabled) error. Update both call sites to use that helper while preserving the existing config-unavailable and HTTP error responses.
🤖 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 `@internal/web/cloud.go`:
- Around line 42-67: The handleCloudConfig method must serialize cloud
configuration updates with the existing cfgMu lock to prevent races with
SaveConfig and other settings handlers. Acquire cfgMu around the SetAutoConnect
call and related status response path, following the established
read-modify-write locking pattern; do not change the existing validation or
error responses.
- Around line 23-36: Guard the fallback path in cloudStatus() against a nil
s.cfg before calling cloud.NewSupervisor, matching the nil-config handling used
by handleCloudConfig. Preserve the existing live cloudSupervisor.Status() path
and return the appropriate offline status without dereferencing a nil
configuration.
In `@web/src/components/CloudBadge.tsx`:
- Line 122: Update the CloudIcon sizing in CloudBadge to use standard Tailwind
h-N and w-N utility classes instead of arbitrary pixel values, while preserving
the current 18px-equivalent dimensions.
In `@web/src/i18n/locales/ko.ts`:
- Around line 1026-1043: Update the Korean cloud localization in the cloud
object: replace “클라우드 릴리” with “클라우드 릴레이” in both title and autoConnectHint,
leaving the other translations unchanged.
---
Nitpick comments:
In `@internal/web/cloud.go`:
- Around line 69-94: Extract the shared CloudSettings
read/modify/save-and-rollback behavior from Supervisor.SetAutoConnect and the
nil-supervisor fallback into a helper such as config.SetCloudAutoConnect(cfg,
enabled) error. Update both call sites to use that helper while preserving the
existing config-unavailable and HTTP error responses.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: e022496d-4f3b-42ad-aa22-5cd4dad509e3
📒 Files selected for processing (20)
desktop/src-tauri/src/sidecar.rsinternal/cloud/client.gointernal/cloud/connector.gointernal/cloud/supervisor.gointernal/cloud/supervisor_test.gointernal/command/cloud.gointernal/command/cloud_test.gointernal/command/web.gointernal/web/cloud.gointernal/web/cloud_test.gointernal/web/server.goweb/src/components/CloudBadge.tsxweb/src/components/SettingsDialog.tsxweb/src/components/Sidebar.tsxweb/src/i18n/locales/en.tsweb/src/i18n/locales/ja.tsweb/src/i18n/locales/ko.tsweb/src/i18n/locales/zh-Hans.tsweb/src/i18n/locales/zh-Hant.tsweb/src/lib/api.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/command/cloud_test.go
- internal/cloud/client.go
| func (s *Server) handleCloudStatus(w http.ResponseWriter, _ *http.Request) { | ||
| writeJSON(w, http.StatusOK, s.cloudStatus()) | ||
| } | ||
|
|
||
| // cloudStatus returns the live supervisor status, or — when no supervisor is | ||
| // wired — an offline snapshot derived from ~/.jcode/cloud.json + config. | ||
| func (s *Server) cloudStatus() cloud.Status { | ||
| if s.cloudSupervisor != nil { | ||
| return s.cloudSupervisor.Status() | ||
| } | ||
| // A never-started supervisor reports exactly the synthesized offline | ||
| // status: credentials re-read from disk, no connector, state "offline". | ||
| return cloud.NewSupervisor(s.cfg, 0, "").Status() | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Can internal/config.Config method calls (e.g. CloudSettings, on a *Config receiver) safely be invoked when the receiver pointer is nil, or would that panic?
💡 Result:
In Go, invoking a method on a nil pointer receiver does not automatically cause a panic [1][2]. Whether the code panics depends entirely on the implementation of the method being called [1][3]. Method calls in Go are effectively syntactic sugar for function calls where the receiver is passed as the first argument [1][4]. Because of this, when you call a method on a nil pointer, the method executes with a nil receiver [1][2]. The outcome depends on what the method does with that receiver: 1. No Panic: If the method does not attempt to access the fields of the receiver or perform any operations that require a valid memory address (dereferencing), the call will succeed [1][3]. You can explicitly handle the nil receiver inside the method to provide "safe" behavior, such as returning a default value or avoiding an action [1][5]. 2. Panic: If the method attempts to access a field, call another method on the nil receiver, or perform any other operation that requires dereferencing the pointer, it will trigger a runtime panic (specifically: runtime error: invalid memory address or nil pointer dereference) [1][6][5]. In summary, the method call itself is valid syntax, but the execution will panic if the method's internal logic is not designed to handle a nil receiver [1][3][6].
Citations:
- 1: https://stackoverflow.com/questions/42238624/calling-a-method-on-a-nil-struct-pointer-doesnt-panic-why-not
- 2: https://claudiuconstantinbogdan.me/articles/nil-value-vs-nil-receiver-go
- 3: https://stackoverflow.com/questions/41547112/golang-panic-on-call-of-nil-objects-method
- 4: https://groups.google.com/g/golang-nuts/c/wcrZ3P1zeAk/m/WI88iQgFMvwJ
- 5: https://georgearisty.dev/posts/golang-methods-on-nil-references/
- 6: https://groups.google.com/g/golang-nuts/c/HgmSxF85MyU
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the relevant files
git ls-files 'internal/web/cloud.go' 'internal/**/cloud*.go' 'internal/**/config*.go'
# Map the cloud.go file structure
ast-grep outline internal/web/cloud.go --view expanded
# Find cloud.NewSupervisor and related config methods
rg -n 'func .*NewSupervisor|type Supervisor|CloudSettings|NewSupervisor\(' internal -g '*.go'Repository: cnjack/jcode
Length of output: 3668
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the relevant sections with line numbers, keeping output small
for f in internal/web/cloud.go $(git ls-files 'internal/**/cloud*.go' 'internal/**/config*.go' | head -n 20); do
[ -f "$f" ] || continue
echo "### $f"
wc -l "$f"
done
echo "### internal/web/cloud.go"
sed -n '1,220p' internal/web/cloud.go
# Try to locate and print the implementation of NewSupervisor and CloudSettings
for f in $(rg -l 'func .*NewSupervisor|CloudSettings' internal -g '*.go' | head -n 20); do
echo "### $f"
sed -n '1,260p' "$f"
doneRepository: cnjack/jcode
Length of output: 50368
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## internal/web/cloud.go"
sed -n '1,220p' internal/web/cloud.go
echo
echo "## candidate cloud files"
for f in $(git ls-files 'internal/**/cloud*.go' 'internal/**/config*.go' | sort); do
echo "### $f"
sed -n '1,260p' "$f"
doneRepository: cnjack/jcode
Length of output: 50368
Potential nil cfg panic in cloudStatus()
cloudStatus() still passes s.cfg into cloud.NewSupervisor(s.cfg, 0, "").Status() without a nil check. If s.cfg can ever be nil, this path can panic through config.CloudAutoConnect(s.cfg) / s.cfg.CloudSettings() inside the supervisor, so the fallback should guard nil the same way handleCloudConfig does.
🤖 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 `@internal/web/cloud.go` around lines 23 - 36, Guard the fallback path in
cloudStatus() against a nil s.cfg before calling cloud.NewSupervisor, matching
the nil-config handling used by handleCloudConfig. Preserve the existing live
cloudSupervisor.Status() path and return the appropriate offline status without
dereferencing a nil configuration.
| func (s *Server) handleCloudConfig(w http.ResponseWriter, r *http.Request) { | ||
| var req cloudConfigPayload | ||
| decoder := json.NewDecoder(io.LimitReader(r.Body, 1<<16)) | ||
| decoder.DisallowUnknownFields() | ||
| if err := decoder.Decode(&req); err != nil { | ||
| writeJSON(w, http.StatusBadRequest, map[string]string{"error": "invalid cloud config: " + err.Error()}) | ||
| return | ||
| } | ||
| if err := decoder.Decode(&struct{}{}); err != io.EOF { | ||
| writeJSON(w, http.StatusBadRequest, map[string]string{"error": "request body must contain one JSON object"}) | ||
| return | ||
| } | ||
| if req.AutoConnect == nil { | ||
| writeJSON(w, http.StatusBadRequest, map[string]string{"error": "auto_connect is required"}) | ||
| return | ||
| } | ||
|
|
||
| enabled := *req.AutoConnect | ||
| if s.cloudSupervisor != nil { | ||
| if err := s.cloudSupervisor.SetAutoConnect(enabled); err != nil { | ||
| writeJSON(w, http.StatusInternalServerError, map[string]string{"error": err.Error()}) | ||
| return | ||
| } | ||
| writeJSON(w, http.StatusOK, s.cloudSupervisor.Status()) | ||
| return | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🌐 Web query:
Does jcode's internal/config.Config type provide its own internal locking for SetCloud/CloudSettings/SaveConfig, independent of caller-supplied mutexes?
💡 Result:
Based on the source code and architectural patterns of the jcode repository, the jcode internal/config.Config type does not implement internal mutex-based locking for operations such as SaveConfig. The configuration management in jcode relies on direct file system operations and standard deserialization/serialization patterns rather than a stateful, thread-safe configuration object with internal locking [1][2][3]. For example, CLI command implementations (such as those handling cloud session configuration) typically load the current configuration into memory, modify the struct fields, and then explicitly call a save function to write the updated configuration to the disk [1]. This approach places the responsibility for concurrency control or state synchronization on the caller, rather than the configuration type itself [1]. Furthermore, while the codebase makes extensive use of synchronization primitives (such as Mutexes) for specific global state management and testing scenarios, these are implemented as external mechanisms (e.g., using std::sync::Mutex or once_cell for initialization) rather than being encapsulated as internal locking mechanisms within the Config type [4][5][6].
Citations:
- 1: https://github.com/1jehuang/jcode/blob/42b9afde/src/cli/commands.rs
- 2: 1jehuang/jcode@5d482ca
- 3: https://github.com/1jehuang/jcode
- 4: MCP tools never reach the agent: locked_tools snapshot races the async MCP-register tokio::spawn 1jehuang/jcode#206
- 5: https://github.com/1jehuang/jcode/blob/42b9afde/src/cli/provider_init_tests.rs
- 6: 1jehuang/jcode@e8818ca
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== Files ==\n'
git ls-files | rg '^(internal/web/(cloud|server)\.go|internal/cloud/supervisor\.go|internal/config/config\.go)$'
printf '\n== Outline: internal/web/cloud.go ==\n'
ast-grep outline internal/web/cloud.go --view expanded || true
printf '\n== Outline: internal/web/server.go ==\n'
ast-grep outline internal/web/server.go --view expanded || true
printf '\n== Outline: internal/cloud/supervisor.go ==\n'
ast-grep outline internal/cloud/supervisor.go --view expanded || true
printf '\n== Outline: internal/config/config.go ==\n'
ast-grep outline internal/config/config.go --view expanded || true
printf '\n== Relevant excerpts ==\n'
for f in internal/web/cloud.go internal/web/server.go internal/cloud/supervisor.go internal/config/config.go; do
if [ -f "$f" ]; then
echo "--- $f ---"
wc -l "$f"
fi
done
printf '\n== Search for cfgMu / SaveConfig / SetAutoConnect ==\n'
rg -n 'cfgMu|SaveConfig|SetAutoConnect|CloudSettings|SetCloud|CloudConfig' internal/web internal/cloud internal/configRepository: cnjack/jcode
Length of output: 35376
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,220p' internal/web/cloud.go
printf '\n--- server.go relevant ---\n'
sed -n '1,180p' internal/web/server.go
printf '\n--- supervisor.go relevant ---\n'
sed -n '1,220p' internal/cloud/supervisor.go
printf '\n--- config.go relevant ---\n'
sed -n '1,260p' internal/config/config.goRepository: cnjack/jcode
Length of output: 28233
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== internal/config/config.go cloud helpers ==\n'
sed -n '440,520p' internal/config/config.go
printf '\n== internal/config/config.go SaveConfig ==\n'
sed -n '1080,1145p' internal/config/config.go
printf '\n== internal/web/cloud_test.go relevant ==\n'
sed -n '1,240p' internal/web/cloud_test.go
printf '\n== internal/cloud/supervisor_test.go relevant ==\n'
sed -n '1,240p' internal/cloud/supervisor_test.goRepository: cnjack/jcode
Length of output: 20098
Cloud config writes should take cfgMu internal/web/cloud.go:42-67 calls s.cloudSupervisor.SetAutoConnect(enabled) directly, but SaveConfig persists the whole *config.Config and other settings handlers already serialize their read-modify-write flow with cfgMu. internal/config.Config only protects Cloud publication; it does not serialize full-config saves, so concurrent cloud toggles or a cloud toggle racing another settings save can drop updates in config.json. Wrap this path with cfgMu or move the lock into the supervisor.
🤖 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 `@internal/web/cloud.go` around lines 42 - 67, The handleCloudConfig method
must serialize cloud configuration updates with the existing cfgMu lock to
prevent races with SaveConfig and other settings handlers. Acquire cfgMu around
the SetAutoConnect call and related status response path, following the
established read-modify-write locking pattern; do not change the existing
validation or error responses.
| title={`${t('cloud.title')} · ${stateLabel}`} | ||
| className="sb-footer-btn flex h-9 w-9 items-center justify-center rounded-[var(--radius-md)] text-[var(--color-muted-foreground)] transition-colors hover:bg-[var(--color-muted)] hover:text-[var(--color-foreground)]" | ||
| > | ||
| <CloudIcon className="h-[18px] w-[18px]" /> |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use standard Tailwind classes for icon sizing.
As per coding guidelines, use Tailwind h-N w-N classes for icon sizing instead of arbitrary values like h-[18px] w-[18px].
♻️ Proposed fix
- <CloudIcon className="h-[18px] w-[18px]" />
+ <CloudIcon className="h-5 w-5" />📝 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.
| <CloudIcon className="h-[18px] w-[18px]" /> | |
| <CloudIcon className="h-5 w-5" /> |
🤖 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 `@web/src/components/CloudBadge.tsx` at line 122, Update the CloudIcon sizing
in CloudBadge to use standard Tailwind h-N and w-N utility classes instead of
arbitrary pixel values, while preserving the current 18px-equivalent dimensions.
Source: Coding guidelines
| // 클라우드 릴리 상태 배지 + 팝오버 (사이드바 하단). | ||
| cloud: { | ||
| badge: '클라우드 상태', | ||
| title: '클라우드 릴리', | ||
| status: { | ||
| online: '연결됨', | ||
| connecting: '연결 중…', | ||
| offline: '연결 끊김', | ||
| error: '연결 오류', | ||
| }, | ||
| notLoggedIn: '로그인되지 않음', | ||
| loginHint: '터미널에서 다음 명령을 실행하여 로그인하세요:', | ||
| autoConnect: '자동 연결', | ||
| autoConnectHint: '시작 시 클라우드 릴리에 자동으로 연결', | ||
| server: '서버', | ||
| device: '이 기기', | ||
| saveFailed: '업데이트 실패: {message}', | ||
| }, |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
"릴리" looks like a typo for "릴레이" (relay).
Both title and autoConnectHint use "클라우드 릴리" — the standard Korean transliteration for "relay" is "릴레이", not "릴리" (which reads as the name "Lily" and has no established meaning here). Both zh-Hans.ts (云中继) and zh-Hant.ts (雲端中繼) correctly use "relay"/"中继", so this looks like an isolated typo in the Korean locale.
📝 Proposed fix
- title: '클라우드 릴리',
+ title: '클라우드 릴레이',
status: {
online: '연결됨',
connecting: '연결 중…',
offline: '연결 끊김',
error: '연결 오류',
},
notLoggedIn: '로그인되지 않음',
loginHint: '터미널에서 다음 명령을 실행하여 로그인하세요:',
autoConnect: '자동 연결',
- autoConnectHint: '시작 시 클라우드 릴리에 자동으로 연결',
+ autoConnectHint: '시작 시 클라우드 릴레이에 자동으로 연결',📝 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.
| // 클라우드 릴리 상태 배지 + 팝오버 (사이드바 하단). | |
| cloud: { | |
| badge: '클라우드 상태', | |
| title: '클라우드 릴리', | |
| status: { | |
| online: '연결됨', | |
| connecting: '연결 중…', | |
| offline: '연결 끊김', | |
| error: '연결 오류', | |
| }, | |
| notLoggedIn: '로그인되지 않음', | |
| loginHint: '터미널에서 다음 명령을 실행하여 로그인하세요:', | |
| autoConnect: '자동 연결', | |
| autoConnectHint: '시작 시 클라우드 릴리에 자동으로 연결', | |
| server: '서버', | |
| device: '이 기기', | |
| saveFailed: '업데이트 실패: {message}', | |
| }, | |
| // 클라우드 릴레이 상태 배지 + 팝오버 (사이드바 하단). | |
| cloud: { | |
| badge: '클라우드 상태', | |
| title: '클라우드 릴레이', | |
| status: { | |
| online: '연결됨', | |
| connecting: '연결 중…', | |
| offline: '연결 끊김', | |
| error: '연결 오류', | |
| }, | |
| notLoggedIn: '로그인되지 않음', | |
| loginHint: '터미널에서 다음 명령을 실행하여 로그인하세요:', | |
| autoConnect: '자동 연결', | |
| autoConnectHint: '시작 시 클라우드 릴레이에 자동으로 연결', | |
| server: '서버', | |
| device: '이 기기', | |
| saveFailed: '업데이트 실패: {message}', | |
| }, |
🤖 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 `@web/src/i18n/locales/ko.ts` around lines 1026 - 1043, Update the Korean cloud
localization in the cloud object: replace “클라우드 릴리” with “클라우드 릴레이” in both
title and autoConnectHint, leaving the other translations unchanged.
- /api/cloud/login|login-status|logout: device-code flow driven from the UI - pairing inbox: pairing.request auto-approve for QR offers, pending approvals via /api/cloud/pairings + approve/deny - /api/cloud/pairing-offer + QR display in CloudBadge popover (5 languages) - shared cloud.Logout/UpdateConfigCloud between CLI and web
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@internal/cloud/auth.go`:
- Around line 55-74: Update UpdateConfigCloud to refresh the shared
Supervisor.s.cfg after saving the cloud configuration, ensuring subsequent
SetAutoConnect operations use the newly persisted Enabled value. Reuse the
existing supervisor configuration state and preserve the current URL and
cloud-setting update behavior.
In `@internal/web/cloud_login.go`:
- Around line 115-164: The start method must preserve a single-login invariant
while RequestDeviceCode is in flight. Add and set an intermediate starting state
under f.mu before releasing the lock, have concurrent start calls treat both
loginPending and starting as already in progress, and transition to loginPending
only after the device-code request succeeds; clear or restore the state on
request failure.
- Around line 186-206: Update the SaveCredentials failure path in the cloud
login flow after RegisterDevice succeeds to perform the same best-effort
server-side device cleanup used by internal/command/login.go. Revoke or
unregister the newly registered device using the returned device identity and
access token, ignore cleanup errors, then call f.fail with the original
SaveCredentials error and return.
In `@web/src/components/CloudBadge.tsx`:
- Around line 82-88: Update the QR container in CloudBadge’s rendered return to
replace the hardcoded bg-white class with the appropriate existing light surface
or dedicated QR quiet-zone CSS token, preserving the current layout and
light-background scanning behavior.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: b72398f9-fe93-44c9-b9d3-e51e0bdebd22
📒 Files selected for processing (21)
internal/cloud/auth.gointernal/cloud/connector.gointernal/cloud/pairing_inbox.gointernal/cloud/pairing_inbox_test.gointernal/cloud/pairings.gointernal/cloud/pairings_test.gointernal/cloud/supervisor.gointernal/command/login.gointernal/web/cloud_login.gointernal/web/cloud_login_test.gointernal/web/cloud_pairings.gointernal/web/cloud_pairings_test.gointernal/web/cloud_test.gointernal/web/server.goweb/src/components/CloudBadge.tsxweb/src/i18n/locales/en.tsweb/src/i18n/locales/ja.tsweb/src/i18n/locales/ko.tsweb/src/i18n/locales/zh-Hans.tsweb/src/i18n/locales/zh-Hant.tsweb/src/lib/api.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- web/src/i18n/locales/zh-Hant.ts
- web/src/i18n/locales/en.ts
- internal/cloud/connector.go
| func UpdateConfigCloud(url string, enabled bool) error { | ||
| cfg, err := config.LoadConfig() | ||
| if err != nil { | ||
| cfg = &config.Config{} | ||
| if data, readErr := os.ReadFile(config.ConfigPath()); readErr == nil { | ||
| _ = json.Unmarshal(data, cfg) | ||
| } | ||
| } | ||
| current := cfg.CloudSettings() | ||
| if url == "" { | ||
| url = current.URL | ||
| } | ||
| cfg.SetCloud(&config.CloudConfig{ | ||
| Enabled: enabled, | ||
| URL: url, | ||
| AutoConnect: current.AutoConnect, | ||
| E2EE: current.E2EE, | ||
| }) | ||
| return config.SaveConfig(cfg) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== SetAutoConnect (in-memory s.cfg writer) =="
rg -nP -A22 'func \(s \*Supervisor\) SetAutoConnect' internal/cloud/supervisor.go
echo "== Does anything refresh s.cfg after login's UpdateConfigCloud? =="
rg -nP 's\.cfg|SetCloud|LoadConfig|UpdateConfigCloud' internal/web/cloud_login.go internal/cloud/supervisor.goRepository: cnjack/jcode
Length of output: 1934
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== internal/web/cloud_login.go relevant sections =="
sed -n '180,330p' internal/web/cloud_login.go
echo
echo "== internal/cloud/supervisor.go relevant sections =="
sed -n '1,260p' internal/cloud/supervisor.go
echo
echo "== config helpers =="
rg -nP -A8 -B4 'func (LoadConfig|SaveConfig|SetCloud|CloudSettings|CloudAutoConnect|CloudE2EE|ConfigPath)\b' internal/config -g '*.go'Repository: cnjack/jcode
Length of output: 16423
Refresh the shared cloud config after login
internal/cloud/auth.go:55-74 writes a fresh config.json, but Supervisor.s.cfg stays stale. A later SetAutoConnect save can write the old Enabled value back to disk and undo the login change.
🤖 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 `@internal/cloud/auth.go` around lines 55 - 74, Update UpdateConfigCloud to
refresh the shared Supervisor.s.cfg after saving the cloud configuration,
ensuring subsequent SetAutoConnect operations use the newly persisted Enabled
value. Reuse the existing supervisor configuration state and preserve the
current URL and cloud-setting update behavior.
| func (f *cloudLoginFlow) start(parentCtx context.Context, rawURL string) (cloudLoginStartResponse, error) { | ||
| baseURL, err := cloud.ValidateCloudURL(rawURL) | ||
| if err != nil { | ||
| return cloudLoginStartResponse{}, err | ||
| } | ||
|
|
||
| f.mu.Lock() | ||
| if f.state == loginPending { | ||
| resp := cloudLoginStartResponse{ | ||
| UserCode: f.userCode, | ||
| VerificationURI: f.verificationURI, | ||
| ExpiresAt: f.expiresAt.UTC().Format(time.RFC3339), | ||
| } | ||
| f.mu.Unlock() | ||
| return resp, nil | ||
| } | ||
| f.mu.Unlock() | ||
|
|
||
| client := f.client(baseURL) | ||
| dc, err := client.RequestDeviceCode(parentCtx, "jcode web "+f.version) | ||
| if err != nil { | ||
| return cloudLoginStartResponse{}, err | ||
| } | ||
|
|
||
| expiresIn := time.Duration(dc.ExpiresIn) * time.Second | ||
| if expiresIn <= 0 { | ||
| expiresIn = 10 * time.Minute | ||
| } | ||
| expiresAt := time.Now().Add(expiresIn) | ||
| interval := time.Duration(dc.Interval) * time.Second | ||
| if f.pollInterval > 0 { | ||
| interval = f.pollInterval | ||
| } | ||
|
|
||
| f.mu.Lock() | ||
| f.state = loginPending | ||
| f.cloudURL = baseURL | ||
| f.userCode = dc.UserCode | ||
| f.verificationURI = dc.VerificationURI | ||
| f.expiresAt = expiresAt | ||
| f.err = "" | ||
| f.mu.Unlock() | ||
|
|
||
| go f.pollAndFinish(parentCtx, client, baseURL, dc.DeviceCode, interval, expiresIn) | ||
| return cloudLoginStartResponse{ | ||
| UserCode: dc.UserCode, | ||
| VerificationURI: dc.VerificationURI, | ||
| ExpiresAt: expiresAt.UTC().Format(time.RFC3339), | ||
| }, nil | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
start() doesn't hold the single-pending invariant across RequestDeviceCode.
The pending check releases the lock before the network call and only sets loginPending afterward, so two concurrent POST /api/cloud/login can both pass the check, both request a device code, and both launch pollAndFinish goroutines (double registration). The UI's loginBusy guard doesn't cover concurrent clients. Consider marking an intermediate "starting" state under the lock before releasing it.
🤖 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 `@internal/web/cloud_login.go` around lines 115 - 164, The start method must
preserve a single-login invariant while RequestDeviceCode is in flight. Add and
set an intermediate starting state under f.mu before releasing the lock, have
concurrent start calls treat both loginPending and starting as already in
progress, and transition to loginPending only after the device-code request
succeeds; clear or restore the state on request failure.
| if err := client.RegisterDevice(ctx, tok.AccessToken, cloud.RegisterDeviceRequest{ | ||
| Name: name, | ||
| Hostname: hostname, | ||
| JcodeVersion: f.version, | ||
| PubKey: pubKey, | ||
| }); err != nil { | ||
| f.fail(err) | ||
| return | ||
| } | ||
| if err := cloud.SaveCredentials(&cloud.Credentials{ | ||
| CloudURL: baseURL, | ||
| DeviceID: tok.DeviceID, | ||
| DeviceToken: tok.AccessToken, | ||
| DeviceName: name, | ||
| PublicKey: pubKey, | ||
| PrivateKey: privKey, | ||
| KeyGen: 1, | ||
| }); err != nil { | ||
| f.fail(err) | ||
| return | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Orphaned server-side device registration if SaveCredentials fails.
RegisterDevice is a non-idempotent remote write that succeeds before SaveCredentials. If the local write fails, the device stays registered on jcloud with no local credential to ever revoke it, and a retry registers yet another device. This mirrors the CLI issue in internal/command/login.go; add the same best-effort cleanup.
🩹 Proposed best-effort cleanup on save failure
if err := cloud.SaveCredentials(&cloud.Credentials{
CloudURL: baseURL,
DeviceID: tok.DeviceID,
DeviceToken: tok.AccessToken,
DeviceName: name,
PublicKey: pubKey,
PrivateKey: privKey,
KeyGen: 1,
}); err != nil {
+ if revokeErr := client.RevokeDevice(ctx, tok.AccessToken); revokeErr != nil {
+ config.Logger().Printf("[cloud] login: failed to revoke orphaned device after save failure: %v", revokeErr)
+ }
f.fail(err)
return
}🤖 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 `@internal/web/cloud_login.go` around lines 186 - 206, Update the
SaveCredentials failure path in the cloud login flow after RegisterDevice
succeeds to perform the same best-effort server-side device cleanup used by
internal/command/login.go. Revoke or unregister the newly registered device
using the returned device identity and access token, ignore cleanup errors, then
call f.fail with the original SaveCredentials error and return.
| return ( | ||
| <div | ||
| className="inline-flex shrink-0 items-center justify-center rounded-[var(--radius-md)] bg-white p-1.5" | ||
| style={{ width: size + 12, height: size + 12 }} | ||
| > | ||
| {src && <img src={src} width={size} height={size} alt="" aria-hidden="true" />} | ||
| </div> |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Route the QR quiet-zone background through a token instead of bg-white.
bg-white hardcodes a color. As per coding guidelines, all component colors must come from CSS custom properties or jcode-ui tokens; never hardcode hex, RGB, #fff, or white. QR scanning does need a light background — use a dedicated light token (or an existing surface/on-light token) so it stays consistent across themes.
♻️ Proposed fix
- className="inline-flex shrink-0 items-center justify-center rounded-[var(--radius-md)] bg-white p-1.5"
+ className="inline-flex shrink-0 items-center justify-center rounded-[var(--radius-md)] bg-[var(--color-qr-bg)] p-1.5"📝 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.
| return ( | |
| <div | |
| className="inline-flex shrink-0 items-center justify-center rounded-[var(--radius-md)] bg-white p-1.5" | |
| style={{ width: size + 12, height: size + 12 }} | |
| > | |
| {src && <img src={src} width={size} height={size} alt="" aria-hidden="true" />} | |
| </div> | |
| return ( | |
| <div | |
| className="inline-flex shrink-0 items-center justify-center rounded-[var(--radius-md)] bg-[var(--color-qr-bg)] p-1.5" | |
| style={{ width: size + 12, height: size + 12 }} | |
| > | |
| {src && <img src={src} width={size} height={size} alt="" aria-hidden="true" />} | |
| </div> |
🤖 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 `@web/src/components/CloudBadge.tsx` around lines 82 - 88, Update the QR
container in CloudBadge’s rendered return to replace the hardcoded bg-white
class with the appropriate existing light surface or dedicated QR quiet-zone CSS
token, preserving the current layout and light-background scanning behavior.
Source: Coding guidelines
…ent inbox (M12) - chat.send project_path/model/effort/goal/attachments pipeline - attachments land in ~/.jcode/inbox/<sid>/ (0600, sanitized, 2MBx5 cap) - capabilities (projects/models/efforts) sealed into sessions upsert - raise API response read cap to 32MB so attachment commands cannot orphan
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
internal/cloud/connector.go (1)
589-706: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueNo cleanup of landed attachments when a later compose step fails.
Step 2 writes attachments to
<inbox>/<sid>/before steps 3-6 (model/effort/mode/goal/chat) run. If any of those later steps fails, the command acks"error"but the already-written attachment files are left orphaned on disk, disconnected from any sent message. This is a minor consistency gap in an otherwise well-ordered "validate before side effects" pipeline; consider either deferring the landing step until immediately before the final chat send, or best-effort removing the just-written files on a later failure.🤖 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 `@internal/cloud/connector.go` around lines 589 - 706, Ensure attachments landed by writeInboxAttachments in execChatSendCompose are cleaned up on any subsequent model, effort, mode, goal, or chat failure; either defer landing until immediately before /api/chat or add best-effort cleanup of the created files while preserving the existing pre-validation behavior.internal/cloud/capabilities.go (1)
39-42: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
standardEffortsglobal slice is aliased, not copied, into the response.
efforts = standardEfforts(line 131) hands out a reference to the package-levelvar standardEffortsslice. Nothing currently mutates the returned slice downstream, but any futureappend/in-place edit on the caller'seffortswould corrupt the shared global for every subsequent call. Aslices.Clone(or manual copy) removes the aliasing risk cheaply.Also applies to: 130-132
🤖 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 `@internal/cloud/capabilities.go` around lines 39 - 42, Update the fallback assignment in the capabilities response around the `standardEfforts` usage to clone the package-level slice before returning it, using `slices.Clone` or an equivalent copy. Preserve the existing effort values while ensuring callers cannot mutate the shared `standardEfforts` backing array.
🤖 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 `@internal/cloud/connector.go`:
- Around line 538-547: Update the empty-input guard in the chat.send handler to
include p.Images alongside p.Text and p.Attachments, so image-only messages
proceed through needsCompose or execChatSendLegacy while genuinely empty
messages still return the existing error.
In `@internal/cloud/inbox.go`:
- Around line 130-138: Update attachmentReferenceList to use the existing
localization mechanism for the attachment reference label instead of hardcoding
“[附件]”. Preserve the current per-reference formatting and path output, while
selecting the label according to the active locale for en, ja, ko, zh-Hans, and
zh-Hant.
- Around line 95-121: Update writeInboxAttachments to eliminate the
os.Stat/os.WriteFile check-then-act race: create each attachment with
os.OpenFile using O_CREATE|O_EXCL and the existing inboxFileMode, retrying with
suffixedName when creation fails because the candidate already exists. Write
decoded[i] to the exclusively created file, close it, and preserve the existing
error wrapping and attachment reference behavior without allowing an existing
file to be truncated.
---
Nitpick comments:
In `@internal/cloud/capabilities.go`:
- Around line 39-42: Update the fallback assignment in the capabilities response
around the `standardEfforts` usage to clone the package-level slice before
returning it, using `slices.Clone` or an equivalent copy. Preserve the existing
effort values while ensuring callers cannot mutate the shared `standardEfforts`
backing array.
In `@internal/cloud/connector.go`:
- Around line 589-706: Ensure attachments landed by writeInboxAttachments in
execChatSendCompose are cleaned up on any subsequent model, effort, mode, goal,
or chat failure; either defer landing until immediately before /api/chat or add
best-effort cleanup of the created files while preserving the existing
pre-validation behavior.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 17ea677b-6a8f-4a3a-8406-6249165dc124
📒 Files selected for processing (8)
internal/cloud/capabilities.gointernal/cloud/client.gointernal/cloud/compose_test.gointernal/cloud/connector.gointernal/cloud/connector_test.gointernal/cloud/inbox.gointernal/cloud/relay.gointernal/cloud/sessions.go
🚧 Files skipped from review as they are similar to previous changes (4)
- internal/cloud/sessions.go
- internal/cloud/relay.go
- internal/cloud/connector_test.go
- internal/cloud/client.go
| // Attachments alone are a valid message (their reference list becomes the | ||
| // text); truly empty input is not. | ||
| if strings.TrimSpace(p.Text) == "" && len(p.Attachments) == 0 { | ||
| return "error", map[string]string{"error": "chat.send: empty text"} | ||
| } | ||
| if p.needsCompose() { | ||
| return c.execChatSendCompose(ctx, cmd, &p) | ||
| } | ||
| return c.execChatSendLegacy(ctx, cmd, &p) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Image-only messages will be wrongly rejected as "empty text".
The guard only checks Text and Attachments, dropping the pre-existing Images check. A chat.send with only Images (a screenshot, no text, no compose attachments) now hits strings.TrimSpace(p.Text) == "" && len(p.Attachments) == 0 and gets acked "error": "chat.send: empty text", even though the comment above explicitly frames "truly empty input" as the only rejection case.
🐛 Proposed fix
- if strings.TrimSpace(p.Text) == "" && len(p.Attachments) == 0 {
+ if strings.TrimSpace(p.Text) == "" && len(p.Images) == 0 && len(p.Attachments) == 0 {
return "error", map[string]string{"error": "chat.send: empty text"}
}📝 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.
| // Attachments alone are a valid message (their reference list becomes the | |
| // text); truly empty input is not. | |
| if strings.TrimSpace(p.Text) == "" && len(p.Attachments) == 0 { | |
| return "error", map[string]string{"error": "chat.send: empty text"} | |
| } | |
| if p.needsCompose() { | |
| return c.execChatSendCompose(ctx, cmd, &p) | |
| } | |
| return c.execChatSendLegacy(ctx, cmd, &p) | |
| } | |
| // Attachments alone are a valid message (their reference list becomes the | |
| // text); truly empty input is not. | |
| if strings.TrimSpace(p.Text) == "" && len(p.Images) == 0 && len(p.Attachments) == 0 { | |
| return "error", map[string]string{"error": "chat.send: empty text"} | |
| } | |
| if p.needsCompose() { | |
| return c.execChatSendCompose(ctx, cmd, &p) | |
| } | |
| return c.execChatSendLegacy(ctx, cmd, &p) | |
| } |
🤖 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 `@internal/cloud/connector.go` around lines 538 - 547, Update the empty-input
guard in the chat.send handler to include p.Images alongside p.Text and
p.Attachments, so image-only messages proceed through needsCompose or
execChatSendLegacy while genuinely empty messages still return the existing
error.
| func writeInboxAttachments(root, sessionID string, atts []chatAttachment, decoded [][]byte) ([]attachmentRef, error) { | ||
| dir := filepath.Join(root, sanitizeInboxName(sessionID)) | ||
| if err := os.MkdirAll(dir, inboxDirMode); err != nil { | ||
| return nil, fmt.Errorf("create inbox dir: %w", err) | ||
| } | ||
| // MkdirAll does not fix the mode of a pre-existing dir. | ||
| if err := os.Chmod(dir, inboxDirMode); err != nil { | ||
| return nil, fmt.Errorf("secure inbox dir: %w", err) | ||
| } | ||
| refs := make([]attachmentRef, 0, len(atts)) | ||
| for i, a := range atts { | ||
| name := sanitizeInboxName(a.Name) | ||
| path := filepath.Join(dir, name) | ||
| for n := 2; ; n++ { | ||
| if _, err := os.Stat(path); os.IsNotExist(err) { | ||
| break | ||
| } | ||
| path = filepath.Join(dir, suffixedName(name, n)) | ||
| } | ||
| if err := os.WriteFile(path, decoded[i], inboxFileMode); err != nil { | ||
| return nil, fmt.Errorf("write attachment %q: %w", a.Name, err) | ||
| } | ||
| _ = os.Chmod(path, inboxFileMode) // umask may have cleared bits | ||
| refs = append(refs, attachmentRef{Name: filepath.Base(path), Path: path}) | ||
| } | ||
| return refs, nil | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
TOCTOU in the collision-avoidance loop (os.Stat then os.WriteFile).
os.Stat+os.IsNotExist to pick a free filename, followed by a separate os.WriteFile, is a classic check-then-act race: anything creating a file at the chosen path between the Stat and the WriteFile (e.g. two attachments in the same command resolving to the same sanitized name via a race, or a retry) causes silent overwrite/data loss since WriteFile truncates unconditionally. Prefer os.OpenFile with O_EXCL to make the existence check and the write atomic.
🔒️ Proposed fix
- for n := 2; ; n++ {
- if _, err := os.Stat(path); os.IsNotExist(err) {
- break
- }
- path = filepath.Join(dir, suffixedName(name, n))
- }
- if err := os.WriteFile(path, decoded[i], inboxFileMode); err != nil {
- return nil, fmt.Errorf("write attachment %q: %w", a.Name, err)
- }
- _ = os.Chmod(path, inboxFileMode) // umask may have cleared bits
+ var f *os.File
+ for n := 2; ; n++ {
+ var oerr error
+ f, oerr = os.OpenFile(path, os.O_WRONLY|os.O_CREATE|os.O_EXCL, inboxFileMode)
+ if oerr == nil {
+ break
+ }
+ if !os.IsExist(oerr) {
+ return nil, fmt.Errorf("create attachment %q: %w", a.Name, oerr)
+ }
+ path = filepath.Join(dir, suffixedName(name, n))
+ }
+ if _, err := f.Write(decoded[i]); err != nil {
+ _ = f.Close()
+ return nil, fmt.Errorf("write attachment %q: %w", a.Name, err)
+ }
+ _ = f.Close()
+ _ = os.Chmod(path, inboxFileMode) // umask may have cleared bits📝 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.
| func writeInboxAttachments(root, sessionID string, atts []chatAttachment, decoded [][]byte) ([]attachmentRef, error) { | |
| dir := filepath.Join(root, sanitizeInboxName(sessionID)) | |
| if err := os.MkdirAll(dir, inboxDirMode); err != nil { | |
| return nil, fmt.Errorf("create inbox dir: %w", err) | |
| } | |
| // MkdirAll does not fix the mode of a pre-existing dir. | |
| if err := os.Chmod(dir, inboxDirMode); err != nil { | |
| return nil, fmt.Errorf("secure inbox dir: %w", err) | |
| } | |
| refs := make([]attachmentRef, 0, len(atts)) | |
| for i, a := range atts { | |
| name := sanitizeInboxName(a.Name) | |
| path := filepath.Join(dir, name) | |
| for n := 2; ; n++ { | |
| if _, err := os.Stat(path); os.IsNotExist(err) { | |
| break | |
| } | |
| path = filepath.Join(dir, suffixedName(name, n)) | |
| } | |
| if err := os.WriteFile(path, decoded[i], inboxFileMode); err != nil { | |
| return nil, fmt.Errorf("write attachment %q: %w", a.Name, err) | |
| } | |
| _ = os.Chmod(path, inboxFileMode) // umask may have cleared bits | |
| refs = append(refs, attachmentRef{Name: filepath.Base(path), Path: path}) | |
| } | |
| return refs, nil | |
| } | |
| func writeInboxAttachments(root, sessionID string, atts []chatAttachment, decoded [][]byte) ([]attachmentRef, error) { | |
| dir := filepath.Join(root, sanitizeInboxName(sessionID)) | |
| if err := os.MkdirAll(dir, inboxDirMode); err != nil { | |
| return nil, fmt.Errorf("create inbox dir: %w", err) | |
| } | |
| // MkdirAll does not fix the mode of a pre-existing dir. | |
| if err := os.Chmod(dir, inboxDirMode); err != nil { | |
| return nil, fmt.Errorf("secure inbox dir: %w", err) | |
| } | |
| refs := make([]attachmentRef, 0, len(atts)) | |
| for i, a := range atts { | |
| name := sanitizeInboxName(a.Name) | |
| path := filepath.Join(dir, name) | |
| var f *os.File | |
| for n := 2; ; n++ { | |
| var oerr error | |
| f, oerr = os.OpenFile(path, os.O_WRONLY|os.O_CREATE|os.O_EXCL, inboxFileMode) | |
| if oerr == nil { | |
| break | |
| } | |
| if !os.IsExist(oerr) { | |
| return nil, fmt.Errorf("create attachment %q: %w", a.Name, oerr) | |
| } | |
| path = filepath.Join(dir, suffixedName(name, n)) | |
| } | |
| if _, err := f.Write(decoded[i]); err != nil { | |
| _ = f.Close() | |
| return nil, fmt.Errorf("write attachment %q: %w", a.Name, err) | |
| } | |
| _ = f.Close() | |
| _ = os.Chmod(path, inboxFileMode) // umask may have cleared bits | |
| refs = append(refs, attachmentRef{Name: filepath.Base(path), Path: path}) | |
| } | |
| return refs, nil | |
| } |
🤖 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 `@internal/cloud/inbox.go` around lines 95 - 121, Update writeInboxAttachments
to eliminate the os.Stat/os.WriteFile check-then-act race: create each
attachment with os.OpenFile using O_CREATE|O_EXCL and the existing
inboxFileMode, retrying with suffixedName when creation fails because the
candidate already exists. Write decoded[i] to the exclusively created file,
close it, and preserve the existing error wrapping and attachment reference
behavior without allowing an existing file to be truncated.
| // attachmentReferenceList renders the reference lines appended to the message | ||
| // text: "[附件] name → path", one per line. | ||
| func attachmentReferenceList(refs []attachmentRef) string { | ||
| var b strings.Builder | ||
| for _, r := range refs { | ||
| fmt.Fprintf(&b, "\n[附件] %s → %s", r.Name, r.Path) | ||
| } | ||
| return b.String() | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Hardcoded Chinese reference label regardless of locale.
attachmentReferenceList always emits "[附件] %s → %s", unconditionally in Chinese, even though the rest of the app is localized across en/ja/ko/zh-Hans/zh-Hant (per the frontend locale files in this stack). Since this text is appended straight into the message the agent (and the user, via the transcript) sees, non-Chinese-speaking users get an unexplained Chinese label in every attachment-bearing message.
🤖 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 `@internal/cloud/inbox.go` around lines 130 - 138, Update
attachmentReferenceList to use the existing localization mechanism for the
attachment reference label instead of hardcoding “[附件]”. Preserve the current
per-reference formatting and path output, while selecting the label according to
the active locale for en, ja, ko, zh-Hans, and zh-Hant.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@AGENTS.md`:
- Line 206: Update the test HOME-isolation rule in AGENTS.md to require setting
both HOME and USERPROFILE to the same temporary directory, or instruct tests to
reuse the existing setHome(t) helper. Preserve the guidance about isolating
tests that resolve config.ConfigDir() and preventing reads from the real
.jcode/AGENTS.md.
- Around line 223-225: Update the E2EE guidance near the “E2EE is the default”
rule to distinguish directions: require outbound relay payloads to use
sealUplink when E2EE is active, while documenting that openDownlink must
continue accepting legacy non-envelope plaintext even when a CEK exists.
Preserve the existing cloud.e2ee:false and pre-CEK behavior without implying
outbound plaintext is permitted during active E2EE.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 971276dc-d1ee-4866-b43e-f661a63d7824
📒 Files selected for processing (5)
AGENTS.mdinternal/cloud/client.gointernal/cloud/connector.gointernal/cloud/supervisor_test.gointernal/command/login.go
🚧 Files skipped from review as they are similar to previous changes (4)
- internal/cloud/supervisor_test.go
- internal/command/login.go
- internal/cloud/client.go
- internal/cloud/connector.go
| - **Don't skip `env.ResolvePath()`.** Raw path concatenation can escape the working directory without warning. | ||
| - **Don't import `internal/tui` from non-TUI packages.** The handler interface is the decoupling boundary. | ||
| - **Don't depend on `jcode-ui` / `jcode-ui-core` via `file:` or `workspace:*`.** Consumers and `packages/jcode-ui`→core must use registry version ranges so publish and local installs match. | ||
| - **Don't let tests read the real HOME.** The pre-push hook runs the full suite; tests must `t.Setenv("HOME", t.TempDir())` whenever code under test resolves `config.ConfigDir()` (a real `~/.jcode/AGENTS.md` breaks `internal/prompts`). |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Document Windows-safe HOME isolation.
Overriding only HOME does not isolate os.UserHomeDir() on Windows; the existing setHome helper also sets USERPROFILE. Update this rule to set both variables to the same temporary directory, or instruct tests to reuse setHome(t), otherwise tests can still read a real ~/.jcode/AGENTS.md on Windows.
Proposed documentation change
-- `t.Setenv("HOME", t.TempDir())` whenever code under test resolves `config.ConfigDir()` (a real `~/.jcode/AGENTS.md` breaks `internal/prompts`).
+- Set both `HOME` and `USERPROFILE` to the same `t.TempDir()` value whenever code under test resolves `config.ConfigDir()` (or reuse the existing `setHome(t)` helper); a real `~/.jcode/AGENTS.md` breaks `internal/prompts`.📝 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.
| - **Don't let tests read the real HOME.** The pre-push hook runs the full suite; tests must `t.Setenv("HOME", t.TempDir())` whenever code under test resolves `config.ConfigDir()` (a real `~/.jcode/AGENTS.md` breaks `internal/prompts`). | |
| - **Don't let tests read the real HOME.** The pre-push hook runs the full suite; tests must set both `HOME` and `USERPROFILE` to the same `t.TempDir()` value (or reuse `setHome(t)`) whenever code under test resolves `config.ConfigDir()` (a real `~/.jcode/AGENTS.md` breaks `internal/prompts`). |
🧰 Tools
🪛 LanguageTool
[style] ~206-~206: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...o publish and local installs match. - Don't let tests read the real HOME. The ...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
🤖 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 `@AGENTS.md` at line 206, Update the test HOME-isolation rule in AGENTS.md to
require setting both HOME and USERPROFILE to the same temporary directory, or
instruct tests to reuse the existing setHome(t) helper. Preserve the guidance
about isolating tests that resolve config.ConfigDir() and preventing reads from
the real .jcode/AGENTS.md.
…ay goal_armed/slash_commands (M14-A) - extract ChatInput/WorkspacePicker/BranchPicker/GoalBanner into packages/jcode-ui/src/product with ProductComposerHost injection (no Redux/fetch/i18n) - web/ rewired via composerHost adapter; desktop behavior unchanged (20-item checklist) - relay: capabilities.slash_commands, chat.send goal_armed -> POST /api/goal start:true, images passthrough tests
…opt-in (M16/M19) - stable machine fingerprint (IOPlatformUUID/machine-id/MachineGuid, sha256) on login/register - per-session sync store (default OFF); cloud-originated sessions always opt-in - sync_default stamps only new sessions; UI CloudSyncToggle + settings switch
…ccess rejection for cloud (M17/M20) - CopyButton for user_code / QR links in CloudBadge popover (5 languages) - ProductComposerHost.allowedModes + modeCeilingHint injection point - connector rejects full_access/bypass aliases for cloud chat.send (mode_not_allowed_for_cloud)
- SettingsView replaces dialog; 14 sections incl. new Cloud tab (login/pairing/QR/auto_connect/logout) - CloudBadge popover slimmed to status + deep link; entries: gear/header/palette/Cmd+, - first web/ vitest (8 tests)
…sion split-reply conflict)
1 participant
Summary
Client side of the jcode ⇄ jcloud device relay (design: cloud repo
docs/17-jcode-device-relay.md; server PR: cnjack/cloud).jcode login/logout— RFC 8628 device code flow; defaulthttps://cloud.j-code.net, self-host HTTPS allowed (http only for localhost dev); credentials in~/.jcode/cloud.json(0600) with X25519 identity keypairinternal/cloud/): starts withjcode webwhen logged in (cloud.auto_connect); heartbeat + long-poll commands → dispatches to the local control plane (/api/chat,/api/stop,/api/approval); WS event pump uploads durable events (idempotent per-session seq, server-seeded resume) and ephemeral token deltas; session index mirror; remote-originated sessions tagged via channel sourceagent_messagedurable synthesis (accumulateagent_textdeltas → finalize onagent_done) so cloud-side history replay keeps assistant textcloud.e2eeswitch; BIP39 recovery phrase;jcode cloudcommand group:pairings/approve/deny/status/key show-phrase/key recover/rotate-key/guidedocs/cloud.mduser guideTestMemoryLoader_EmptyDirHOME isolation (was failing pre-push on real machines)Test plan (all green)
go test ./...green (incl.-raceon internal/cloud), golangci-lint 0 issuesj7device login (39 assertions),j8relay loop with real jcode web + mockllm (34),j9E2EE with server-side zero-plaintext proof (31)Summary by CodeRabbit
jcode login,jcode logout, andjcode cloudfor pairing requests and E2EE key management (show/recover/rotate via recovery phrase).sourcelabel for externally-originated messages.