Repository navigation
feat: add execution context routing - #1
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 461d2fce83
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if shell == "" { | ||
| shell = defaultShell() | ||
| } | ||
| ctx, cancel := context.WithTimeout(context.Background(), timeout) |
There was a problem hiding this comment.
When an execution_context=desktop or user request is canceled or its client disconnects before timeout_seconds, the bridge starts the command under context.Background(), so neither the canceled request nor the closed socket stops it. Unlike the service execution path, the handler can remain blocked and the process group continues running for up to the configured timeout (potentially an hour under the default policy); propagate cancellation/disconnection to the bridge and kill the process group.
Useful? React with 👍 / 👎.
| lines := strings.Split(strings.TrimSpace(string(data)), "\n") | ||
| for _, line := range lines { | ||
| parts := strings.SplitN(line, ":", 3) | ||
| if len(parts) == 3 && parts[2] != "" { | ||
| return parts[2] | ||
| } |
There was a problem hiding this comment.
On cgroup v1 or hybrid Linux systems, /proc/<pid>/cgroup contains multiple controller entries and the session-N.scope path is commonly in the name=systemd entry rather than the first entry. Returning the first nonempty path makes Run conclude that a correctly launched graphical bridge has no login session and refuse to start, disabling all desktop/user routing on those systems; scan all hierarchy paths or select the systemd hierarchy.
Useful? React with 👍 / 👎.
| if runtimeDir := strings.TrimSpace(os.Getenv("XDG_RUNTIME_DIR")); runtimeDir != "" { | ||
| return filepath.Join(runtimeDir, "cycomagent-session.sock") | ||
| } | ||
| return filepath.Join("/run/user", strconv.Itoa(os.Getuid()), "cycomagent-session.sock") |
There was a problem hiding this comment.
When one Unix account has multiple simultaneous graphical login sessions, XDG_RUNTIME_DIR and /run/user/<uid> are shared by all of them, so every bridge resolves to the same socket. The second session's bridge therefore exits as “already running,” and desktop commands continue to inherit the first session's display, D-Bus, cgroup, and authorization context rather than the intended active session; use session-specific endpoints and select the appropriate active bridge.
Useful? React with 👍 / 👎.
| if err := json.NewDecoder(io.LimitReader(conn, 24<<20)).Decode(out); err != nil { | ||
| return fmt.Errorf("read desktop session bridge response: %w", err) |
There was a problem hiding this comment.
When a desktop command emits control characters, quotes, or backslashes near the allowed output limit, JSON escaping can make the response much larger than the raw stdout/stderr buffers; for example, NUL bytes expand to six bytes each. The bridge allows up to 10 MiB independently for stdout and stderr, but the client truncates the encoded response stream at 24 MiB, so even otherwise valid output can produce an unexpected EOF instead of the promised result and truncation metadata; bound the encoded payload or raise/derive this limit from the worst-case escaped size.
Useful? React with 👍 / 👎.
| for range ticker.C { | ||
| if processAlive(pid) { | ||
| continue |
There was a problem hiding this comment.
When a desktop job leaves descendants running after its shell exits, such as sleep 60 & or a GUI program that daemonizes, this monitor tests only the shell PID and immediately records the job as exited-external even though its returned PGID is still alive. That makes job status and health counts incorrect and permits pruning the durable handle/log while the actual session process continues; monitor the process group (and preserve its identity against reuse) until the whole group exits.
Useful? React with 👍 / 👎.
1 participant
Summary
execution_context=auto|service|user|desktop|systemto process executionsession-N.scopesystemexecutionValidation
go test ./...go vet ./...make releasepackage validationpowerprofilesctlchild ran insession-7.scope, switched Performance -> Balanced -> Performance without Polkit AccessDenied