fix(cli): hoist preconditions above conversation resolve (DEF-48) - #1402
Conversation
sendMessageViaConversation resolved the conversation before checking preconditions that could cause early returns. ResolveConversation can CREATE a conversation row, so when a precondition failed afterward, the row survived with no message — orphaning it. Two preconditions are hoisted above resolve: 1. @agent path: build and validate the StructuredMessage. buildStructuredMessage does not need ConversationID, and ValidateLegacyMessage does not check it (DEF-41 removed that check), so the message is fully validatable before resolve. 2. @email path: the SCION_AGENT_NAME agent-context check reads one environment variable and depends on nothing from the resolve. A human CLI context (empty SCION_AGENT_NAME) cannot send via @email, and failing after resolve orphans the row identically. The remaining post-resolve return (:773, "unsupported ref kind") is unreachable: RefConversation and RefThread are gated at the CLI entry point (cmd/message.go:154-155) and never reach this function. Deleted stale comment (4 lines): the block above the @email branch claimed conv:<uuid> and #<thread> references "reach this function" and are "persisted with the conversation_id." That was false -- both are gated at the CLI entry point (:154-155) and never arrive. The comment directly contradicted the unreachability comment 25 lines below it. Removing the false account; the accurate one is kept. AC-D-4: TestSendMessageViaConversation_ValidationBeforeResolve proves zero resolves when @agent validation fails. TestSendMessageViaConversation_EmailPreconditionBeforeResolve proves zero resolves when @email precondition fails. Both red before fix, green after. Per-file deletion counts: cmd/message.go: 15 deletions (11 mechanical, 4 stale comment) No assertions deleted; no coverage successors owed.
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request hoists the validation of @agent messages and the precondition check for @email references to run before resolving the conversation, preventing orphaned conversation rows when validation or preconditions fail. It also adds unit tests to verify this behavior. The reviewer suggests hoisting the @email precondition check even further to the very beginning of sendMessageViaConversation to avoid unnecessary network calls to resolve the sender's identity when SCION_AGENT_NAME is empty.
| if ref.Kind == messaging.RefEmail { | ||
| emailSenderAgent = os.Getenv("SCION_AGENT_NAME") | ||
| if emailSenderAgent == "" { | ||
| return fmt.Errorf("sending messages to users via @<email> is only supported from within an agent container (SCION_AGENT_NAME not set)") | ||
| } | ||
| } |
There was a problem hiding this comment.
The precondition check for @email references (SCION_AGENT_NAME == "") is currently executed after resolveSenderIdentity(hubCtx) (line 675).
When SCION_AGENT_NAME is empty (e.g., in a human CLI context), resolveSenderIdentity will attempt to resolve the user's identity by making a network call to /api/v1/auth/me with a 5-second timeout. This means that if a human user attempts to send a message to an email reference, the CLI will unnecessarily make a network call (and potentially hang or fail) before finally returning the "only supported from within an agent container" error.
To prevent this unnecessary network overhead and improve responsiveness, we should hoist this check to the very beginning of sendMessageViaConversation (before resolveSenderIdentity).
if ref.Kind == messaging.RefEmail {
emailSenderAgent = os.Getenv("SCION_AGENT_NAME")
}Not a DEF-48 correctness fix — the precondition was already above ResolveConversation and could not orphan a row. This is a reviewer- prompted reorder: SCION_AGENT_NAME depends only on the environment and nothing computed by this function, so evaluating it after resolveSenderIdentity (which makes a network call via Auth().Me()) wastes a round trip on a guaranteed failure in human CLI context. Move the @email precondition block above resolveSenderIdentity. The @agent validation block stays where it is — it needs `sender`. Per-file deletion counts: cmd/message.go: 11 deletions (7 relocated, 4 comment replaced) No assertions deleted; no coverage successors owed.
1 participant
sendMessageViaConversation resolved the conversation before checking preconditions that could return early. ResolveConversation can CREATE a conversation row, so a precondition failing afterward left the row with no message.
Two preconditions hoisted above the resolve:
The third post-resolve return ("unsupported ref kind") is unreachable: conv: and # are gated at CLI entry (cmd/message.go:154-155).
Also removes a 4-line stale comment that claimed conv:/#thread reach this function and are persisted here. They never arrive; it contradicted the unreachability comment 25 lines below.
Tests: TestSendMessageViaConversation_ValidationBeforeResolve and TestSendMessageViaConversation_EmailPreconditionBeforeResolve, both asserting zero resolves. Verified red with the fix reverted and green with it, by the reviewer.
2 files, +110/-15. cmd/message.go: 15 deletions (11 mechanical relocation/rename, 4 stale comment). No assertions deleted