feat(shellv2): add interactive portal support and output deduplication#1825
feat(shellv2): add interactive portal support and output deduplication#1825KCarretto wants to merge 10 commits intofeat/shellv2-backendfrom
Conversation
Co-authored-by: KCarretto <16250309+KCarretto@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
| } | ||
| if err := h.mux.Publish(ctx, topicIn, mote); err != nil { | ||
| slog.ErrorContext(ctx, "failed to publish to portal", "error", err) | ||
| // Fallback to just DB? Or error? We continue to DB write. |
There was a problem hiding this comment.
Unset the active portal id here since it's likely closed or there is some other error. Before doing so perhaps query again to see if it's closed.
There was a problem hiding this comment.
Addressed. I now call checkPortal() immediately on publish failure. If it confirms the portal is gone (or if it thinks it's still there but we just failed), I cleanup the portal state to fallback to DB-only mode.
…ing up portal state Co-authored-by: KCarretto <16250309+KCarretto@users.noreply.github.com>
tavern/internal/http/shell_v2.go
Outdated
| case msg := <-wsReadCh: | ||
| slog.DebugContext(ctx, "ShellV2 Recv", "msg", string(msg)) | ||
|
|
||
| var req struct { |
There was a problem hiding this comment.
Please explicitly define this as a WebsocketMessage struct at the top
There was a problem hiding this comment.
Added WebsocketMessage struct definition and updated usage.
Co-authored-by: KCarretto <16250309+KCarretto@users.noreply.github.com>
tavern/internal/http/shell_v2.go
Outdated
| Save(ctx) | ||
| if err != nil { | ||
| slog.ErrorContext(ctx, "failed to create shell task", "error", err) | ||
| sendToWS(map[string]string{ |
There was a problem hiding this comment.
Use our new WebsocketMessage here and everywhere we write to or read from the websocket
There was a problem hiding this comment.
Updated wsWriteCh to use WebsocketMessage and refactored all sendToWS calls to use the struct.
Co-authored-by: KCarretto <16250309+KCarretto@users.noreply.github.com>
tavern/internal/http/shell_v2.go
Outdated
| } | ||
|
|
||
| sendToWS(WebsocketMessage{ | ||
| Type: "OUTPUT", |
There was a problem hiding this comment.
Create a WebsocketMessageKind enum and use it everywhere in this file instead of hard coding values
There was a problem hiding this comment.
Refactored WebsocketMessage to use WebsocketMessageKind enum constants (EXECUTE, OUTPUT, ERROR).
Co-authored-by: KCarretto <16250309+KCarretto@users.noreply.github.com>
| portalCancel = cancel | ||
|
|
||
| // Forward messages | ||
| go func() { |
There was a problem hiding this comment.
Move these go routines to their own functions for improved readability
There was a problem hiding this comment.
Refactored goroutines into wsReader, wsWriter, and portalForwarder methods for better readability.
Co-authored-by: KCarretto <16250309+KCarretto@users.noreply.github.com>
…rtal comms Co-authored-by: KCarretto <16250309+KCarretto@users.noreply.github.com>
tavern/internal/http/shell_v2.go
Outdated
| // Let's assume we maintain a map of readers if we expect multiple streams, | ||
| // or just one if the portal traffic is single-stream. | ||
| // For a shell session, it's usually one stream from the agent. | ||
| readers := make(map[string]*stream.OrderedReader) |
There was a problem hiding this comment.
We may receive output from different stream ids, this is for "multiplayer" sessions where multiple users might send input to the same session. When we receive output from a different session id, we still want to write it to the websocket but let's highlight that it's from another user. For now we can represent this using a new message kind (OUTPUT_FROM_OTHER_USER or something like that)
There was a problem hiding this comment.
Added OUTPUT_OTHER_USER message kind. Logic: if the mote's Stream ID matches our local stream ID, it's OUTPUT. If it differs, it's OUTPUT_OTHER_USER.
Co-authored-by: KCarretto <16250309+KCarretto@users.noreply.github.com>
| // staleTimeout is the duration to wait for the next expected sequence ID before erroring if other messages are arriving. | ||
| func NewOrderedReader(receiver ReceiverFunc, options ...func(*OrderedReader)) *OrderedReader { | ||
| func NewOrderedReader(options ...func(*OrderedReader)) *OrderedReader { | ||
| reader := &OrderedReader{ |
There was a problem hiding this comment.
Revert your changes to the portal package, you do not have permission to make modifications here
There was a problem hiding this comment.
Reverted changes to tavern/portals/stream as requested. I have adapted ShellV2Handler to use OrderedReader with its existing ReceiverFunc interface by bridging it with a per-stream channel and a goroutine. This allows us to use OrderedReader without modifying the portal package.
Co-authored-by: KCarretto <16250309+KCarretto@users.noreply.github.com>
| defer cleanupPortal() | ||
|
|
||
| // Initial check for portal | ||
| checkPortal := func() { |
There was a problem hiding this comment.
move this to a utility function
There was a problem hiding this comment.
Refactored checkPortal logic into a method ShellV2Handler.checkPortal.
Co-authored-by: KCarretto <16250309+KCarretto@users.noreply.github.com>
1 participant
portal.prototo includeoutputandtask_idinShellPayload.ShellV2Handlerintavern/internal/http/shell_v2.go:PR created automatically by Jules for task 3518336442341510418 started by @KCarretto