fix(cloud): create sessions before legacy sends - #168
Merged
Conversation
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
📝 WalkthroughWalkthroughThe connector now explicitly creates or focuses local sessions before chat submission, validates returned session IDs, and propagates those IDs in acknowledgements. Cloud session upserts also include UTC-normalized activity timestamps for ordering without decrypting metadata. ChangesSession synchronization
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant CloudConnector
participant LocalSessionsAPI
participant LocalChatAPI
CloudConnector->>LocalSessionsAPI: Create or focus session
LocalSessionsAPI-->>CloudConnector: Return session_id
CloudConnector->>LocalChatAPI: Submit chat with session_id
LocalChatAPI-->>CloudConnector: Return matching session_id
CloudConnector-->>CloudConnector: Acknowledge result.session_id
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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/cloud/connector.go (1)
852-859: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winValidate the compose chat response session ID before acknowledging success.
Unlike the legacy path, compose accepts any successful
/api/chatresponse and returnssidwithout checking the returnedsession_id. A mismatched or missing response ID can acknowledge a session that did not receive the message. Parse and require a matchingsession_idhere as well.🤖 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 852 - 859, Update the compose `/api/chat` success path in the surrounding connector method to parse the response body and require a non-empty `session_id` matching `sid` before returning success. Reuse the existing response-validation behavior or helper from the legacy path if available, and return an error result for missing or mismatched IDs instead of acknowledging the session.
🤖 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/sessions.go`:
- Around line 53-58: Move LastActivityAt from the plaintext SessionUpsert fields
into the value sealed by sealUplink, preserving its use for ordering after
openDownlink. Update internal/cloud/relay.go lines 36-39,
internal/cloud/crypto_wiring_test.go lines 116-130, and docs/cloud.md lines
47-50 to reflect and verify that all session activity metadata is encrypted when
a CEK exists; these sites require corresponding contract, test, and
documentation updates.
---
Outside diff comments:
In `@internal/cloud/connector.go`:
- Around line 852-859: Update the compose `/api/chat` success path in the
surrounding connector method to parse the response body and require a non-empty
`session_id` matching `sid` before returning success. Reuse the existing
response-validation behavior or helper from the legacy path if available, and
return an error result for missing or mismatched IDs instead of acknowledging
the session.
🪄 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 Plus
Run ID: b28be72e-d902-4fa4-8f15-7cc585df611c
📒 Files selected for processing (8)
docs/cloud.mdinternal/cloud/connector.gointernal/cloud/connector_test.gointernal/cloud/crypto_wiring_test.gointernal/cloud/relay.gointernal/cloud/sessions.gointernal/cloud/sync_filter_test.gointernal/web/concurrency_test.go
Comment on lines
53
to
58
| upserts = append(upserts, SessionUpsert{ | ||
| SessionID: m.UUID, | ||
| Status: status, | ||
| Meta: c.sealUplink(metaJSON), | ||
| SessionID: m.UUID, | ||
| Status: status, | ||
| Meta: c.sealUplink(metaJSON), | ||
| LastActivityAt: sessionLastActivityAt(m), | ||
| }) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files | rg '(^|/)(sessions|relay|crypto_wiring_test)\.go$|docs/cloud\.md$' || true
echo "== sessions.go outline =="
ast-grep outline internal/cloud/sessions.go --view compact || true
echo "== sessions.go relevant sections =="
cat -n internal/cloud/sessions.go | sed -n '1,140p'
echo "== relay.go relevant sections =="
cat -n internal/cloud/relay.go | sed -n '1,120p'
echo "== crypto_wiring_test.go relevant sections =="
cat -n internal/cloud/crypto_wiring_test.go | sed -n '1,180p'
echo "== docs/cloud.md relevant sections =="
cat -n docs/cloud.md | sed -n '1,120p'
echo "== sealUplink/openDownlink definitions/usages =="
rg -n "func .*sealUplink|func .*openDownlink|sealUplink|openDownlink" internal/cloud -SRepository: cnjack/jcode
Length of output: 23948
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== connector sealing implementation =="
cat -n internal/cloud/connector.go | sed -n '250,320p'
echo "== package comments for internal/cloud =="
cat -n internal/cloud/*.go | sed -n '1,25p'
echo "== deterministic read-only check: plaintext fields outside sealUplink calls =="
python3 - <<'PY'
from pathlib import Path
for path in [Path("internal/cloud/sessions.go"), Path("internal/cloud/relay.go"), Path("internal/cloud/crypto_wiring_test.go"), Path("docs/cloud.md")]:
text = path.read_text()
print(f"\n--- {path} ---")
for needle in [
"LastActivityAt",
"last_activity_at",
"intentionally plaintext for cloud-side ordering",
"want plaintext UTC timestamp",
"可.加密的\n元数据，以及可选的明文字段 `last_activity_at`",
]:
idx = text.find(needle)
print(f"{needle!r}: {'FOUND' if idx >= 0 else 'MISSING'}")
PYRepository: cnjack/jcode
Length of output: 5020
Do not send session activity metadata outside the E2EE envelope.
LastActivityAt is sent in plaintext with an active CEK, and the cipher-enabled test documents that the cloud receives it unsealed. This violates the internal/cloud E2EE relay payload rule that relay payloads must use sealUplink/openDownlink once a CEK exists. Move the ordering field under the sealed payload or update the policy/code/tests to match an approved plaintext ordering contract.
internal/cloud/sessions.go#L53-L58internal/cloud/relay.go#L36-L39internal/cloud/crypto_wiring_test.go#L116-L130docs/cloud.md#L47-L50
📍 Affects 4 files
internal/cloud/sessions.go#L53-L58(this comment)internal/cloud/relay.go#L36-L39internal/cloud/crypto_wiring_test.go#L116-L130docs/cloud.md#L47-L50
🤖 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/sessions.go` around lines 53 - 58, Move LastActivityAt from
the plaintext SessionUpsert fields into the value sealed by sealUplink,
preserving its use for ordering after openDownlink. Update
internal/cloud/relay.go lines 36-39, internal/cloud/crypto_wiring_test.go lines
116-130, and docs/cloud.md lines 47-50 to reflect and verify that all session
activity metadata is encrypted when a CEK exists; these sites require
corresponding contract, test, and documentation updates.
Source: Coding guidelines
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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`:
- Around line 223-228: Update the uplink sealing flow around sealUplink so any
sealing failure rejects or drops the content-bearing upload instead of logging
the error and sending the original plaintext. Preserve plaintext transmission
only for the explicitly allowed pre-CEK or cloud.e2ee:false cases, and ensure
callers handle the rejected upload without bypassing 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
Comment on lines
+223
to
+228
| - **E2EE is the default.** Session content, metadata, command bodies/results, | ||
| and other content-bearing relay payloads are sealed once a CEK exists; gray | ||
| plaintext content is only for pre-CEK or `cloud.e2ee:false`. Plaintext is | ||
| limited to routing metadata required by the orchestrator: ids, seq, kind, | ||
| timestamps (including `last_activity_at`), and online state. Never add a | ||
| content-bearing uplink path that bypasses `sealUplink`/`openDownlink`. |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Fail closed when E2EE sealing fails
This says content-bearing payloads are sealed whenever a CEK exists, but internal/cloud/connector.go:270-281 currently logs the sealing error and sends the original plaintext. A transient crypto failure can therefore expose session content despite cloud.e2ee being enabled. Make sealUplink reject/drop the upload on sealing failure, or explicitly document this exception and its security implications.
🤖 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` around lines 223 - 228, Update the uplink sealing flow around
sealUplink so any sealing failure rejects or drops the content-bearing upload
instead of logging the error and sending the original plaintext. Preserve
plaintext transmission only for the explicitly allowed pre-CEK or
cloud.e2ee:false cases, and ensure callers handle the rejected upload without
bypassing E2EE.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
chat.sendwithout a session ID, avoiding active-engine misrouting.last_activity_atmetadata for device-session mirror ordering while keeping session metadata encrypted.Validation
make build-web && go test ./internal/cloud ./internal/webSummary by CodeRabbit
New Features
Bug Fixes
Documentation