Feature/devcontainers#63
Open
kiuma wants to merge 3 commits into
Open
Conversation
Reviewer's GuideAdds devcontainer awareness and controls to the GNOME Shell Docker extension, improves CLI/terminal handling and startup performance, switches the settings schema namespace, and tightens the build and packaging configuration. Sequence diagram for starting a devcontainer from the GNOME Shell menusequenceDiagram
actor User
participant TopPanel as GnomeShellTopPanel
participant DockerMenu
participant DockerSubMenu
participant StartItem as DevcontainerStartMenuItem
participant DockerModule
participant DevProc as runDevcontainerProcess
participant DevCLI as devcontainer_CLI
participant Shell as UserShell
User ->> TopPanel: Open extension menu
TopPanel ->> DockerMenu: show()
DockerMenu ->> DockerModule: getContainers()
DockerModule -->> DockerMenu: containers with devcontainer info
DockerMenu ->> DockerSubMenu: create for each container
User ->> DockerSubMenu: Select devcontainer container
User ->> StartItem: Activate Start
StartItem ->> DockerModule: runDevcontainerUp(localFolder)
DockerModule ->> TopPanel: Main.notify("Devcontainer", "Starting…")
DockerModule ->> DevProc: runDevcontainerProcess("devcontainer up --workspace-folder localFolder", localFolder)
DevProc ->> Shell: spawn $SHELL -i -l -c "devcontainer up ... > log 2>&1"
Shell ->> DevCLI: devcontainer up --workspace-folder localFolder
DevCLI -->> Shell: exit status
Shell -->> DevProc: process finished
alt success
DevProc -->> DockerModule: resolve(logFile)
DockerModule ->> DockerModule: delete logFile
DockerModule ->> TopPanel: Main.notify("Devcontainer", "started")
else failure
DevProc -->> DockerModule: reject(logFile)
DockerModule ->> DockerModule: openTerminalWithLog(logFile, localFolder)
DockerModule ->> Shell: spawn terminal showing log and shell
end
Class diagram for updated Docker menu and devcontainer integrationclassDiagram
class DockerModule {
<<module>>
+hasDocker() bool
+hasPodman() bool
+hasDevcontainer() bool
+detectDevcontainerCli() bool
+openTerminalAtFolder(folderPath string) void
+runDevcontainerUp(localFolder string) void
+runDevcontainerRecreate(localFolder string) void
+isUserInDockerGroup() bool
+getContainers() Array
}
class DockerMenu {
-settings
-_timeout
+_init() void
+_checkServices() void
+_checkDockerRunning() void
+_checkUserInDockerGroup() void
+_refreshCount() void
}
class DockerSubMenu {
-_parentMenu
+_init(compose object, devcontainer object, containerName string, containerStatusMessage string, parentMenu object, closePopup function) void
-_setupDevcontainerName(devcontainerName string) void
+_getTopMenu() object
}
class DockerMenuItem {
+_init(containerName string, actions Array, icon object, closePopup function) void
}
class DevcontainerStartMenuItem {
+_init(localFolder string, icon object, closePopup function) void
}
class DevcontainerRecreateMenuItem {
+_init(localFolder string, icon object, closePopup function) void
}
class DevcontainerInfo {
+name string
+localFolder string
+configFile string
}
DockerMenu --> DockerModule : uses
DockerMenu --> DockerSubMenu : creates
DockerSubMenu --> DockerMenuItem : creates
DockerSubMenu --> DevcontainerStartMenuItem : creates
DockerSubMenu --> DevcontainerRecreateMenuItem : creates
DockerSubMenu --> DevcontainerInfo : aggregates
DevcontainerStartMenuItem --> DockerModule : calls runDevcontainerUp()
DevcontainerRecreateMenuItem --> DockerModule : calls runDevcontainerRecreate()
DockerModule ..> DevcontainerInfo : returns
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- The temporary log path in runDevcontainerProcess uses only the workspace folder basename (e.g. /tmp/devcontainer-foo.log), which can collide across multiple workspaces or concurrent runs; consider including a PID or timestamp in the filename to avoid overwriting logs.
- Terminal detection logic is duplicated between openTerminalAtFolder and devcontainerTerminalArgv; extracting a shared helper that returns the selected terminal and base argv would reduce repetition and keep behavior in sync.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The temporary log path in runDevcontainerProcess uses only the workspace folder basename (e.g. /tmp/devcontainer-foo.log), which can collide across multiple workspaces or concurrent runs; consider including a PID or timestamp in the filename to avoid overwriting logs.
- Terminal detection logic is duplicated between openTerminalAtFolder and devcontainerTerminalArgv; extracting a shared helper that returns the selected terminal and base argv would reduce repetition and keep behavior in sync.
## Individual Comments
### Comment 1
<location path="src/docker.js" line_range="189-190" />
<code_context>
+ argv = ["ptyxis", "--working-directory", folderPath];
+ } else if (validTerminals["gnome-terminal"]) {
+ argv = ["gnome-terminal", "--working-directory", folderPath];
+ } else if (validTerminals["x-terminal-emulator"]) {
+ argv = ["sh", "-c", `x-terminal-emulator -e sh -c 'cd "${folderPath}"; exec $SHELL'`];
+ } else {
+ logError(
</code_context>
<issue_to_address>
**🚨 issue (security):** Folder paths containing quotes or shell metacharacters will break the x-terminal-emulator command and can be abused.
In the x-terminal-emulator branch, `folderPath` is embedded inside a single-quoted shell string without escaping, so quotes or metacharacters can both break `cd` and enable command injection. Please either:
- Wrap `folderPath` with `GLib.shell_quote(folderPath)` while keeping the surrounding single quotes, or
- Drop the nested `sh -c` and build `argv` as `['x-terminal-emulator', '-e', 'sh', '-c', 'cd ' + GLib.shell_quote(folderPath) + '; exec $SHELL']`.
This ensures arbitrary paths (spaces, quotes, etc.) are passed safely to the shell.
</issue_to_address>
### Comment 2
<location path="src/docker.js" line_range="379" />
<code_context>
+ * @param {String} localFolder
+ */
+const openTerminalWithLog = (logFile, localFolder) => {
+ const shellCmd = `cat "${logFile}"; rm -f "${logFile}"; cd "${localFolder}"; exec $SHELL`;
+ const argv = devcontainerTerminalArgv(shellCmd);
+ if (!argv) return;
</code_context>
<issue_to_address>
**🚨 issue (security):** Unescaped localFolder in the log terminal shell command can break for paths with quotes or special characters.
`openTerminalWithLog` builds a shell command with `localFolder` interpolated directly. If `localFolder` includes quotes or shell metacharacters (e.g. `"`, `;`, `&&`), the `cd` may fail or execute unintended commands. Please wrap `localFolder` with `GLib.shell_quote` when constructing `shellCmd` and adjust the surrounding quotes so it works safely for arbitrary folder names.
</issue_to_address>
### Comment 3
<location path="src/docker.js" line_range="259" />
<code_context>
- const _userName = GLib.get_user_name();
- let _userGroups = GLib.ByteArray.toString(
- GLib.spawn_command_line_sync("groups " + _userName)[1],
+const getUserShell = () => GLib.getenv("SHELL") || "sh";
+
+/**
</code_context>
<issue_to_address>
**nitpick (bug_risk):** The getUserShell JSDoc promises an absolute path, but the implementation may return a bare command name.
If `SHELL` is unset, this returns just `"sh"`, which contradicts the promise of an absolute path. Please either change the fallback to an absolute path (e.g. `/bin/sh`) or relax the JSDoc so it no longer guarantees an absolute path, to keep behavior and documentation aligned.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
kiuma
force-pushed
the
feature/devcontainers
branch
from
9f6f810 to
f66d741
Compare
Member
Author
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- In
DockerMenu._checkDockerRunningyou still referenceDocker.hasPodmanas a property instead of callingDocker.hasPodman(), which will now always be truthy and skip the Docker-running check when Podman is absent. - The
shellCmdstrings built forrunDevcontainerProcess/runDevcontainerUp/runDevcontainerRecreateinterpolatelocalFolderdirectly into a quoted argument; consider usingGLib.shell_quote(localFolder)(and building the command accordingly) to avoid breakage or injection issues with paths containing quotes or shell metacharacters. - The 5‑second startup timeout stored in
this._timeoutis never cleared on destruction ofDockerMenu; it would be safer to track and remove this source in the destroy/disable path to avoid callbacks firing after the extension is disabled.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `DockerMenu._checkDockerRunning` you still reference `Docker.hasPodman` as a property instead of calling `Docker.hasPodman()`, which will now always be truthy and skip the Docker-running check when Podman is absent.
- The `shellCmd` strings built for `runDevcontainerProcess` / `runDevcontainerUp` / `runDevcontainerRecreate` interpolate `localFolder` directly into a quoted argument; consider using `GLib.shell_quote(localFolder)` (and building the command accordingly) to avoid breakage or injection issues with paths containing quotes or shell metacharacters.
- The 5‑second startup timeout stored in `this._timeout` is never cleared on destruction of `DockerMenu`; it would be safer to track and remove this source in the destroy/disable path to avoid callbacks firing after the extension is disabled.
## Individual Comments
### Comment 1
<location path="src/docker.js" line_range="337-346" />
<code_context>
+ * @param {String} localFolder Workspace folder (used to name the log file)
+ * @return {Promise<string>}
+ */
+const runDevcontainerProcess = (shellCmd, localFolder) => {
+ const folderName = GLib.path_get_basename(localFolder);
+ // Include a timestamp to avoid collisions across concurrent runs or
+ // multiple workspaces that share the same basename.
+ const logFile = `${GLib.get_tmp_dir()}/devcontainer-${folderName}-${Date.now()}.log`;
+ const userShell = getUserShell();
+ const shellFlags = getLoginShellFlags();
+
+ return new Promise((resolve, reject) => {
+ try {
+ const proc = new Gio.Subprocess({
+ argv: [userShell, ...shellFlags, "-c", `${shellCmd} > "${logFile}" 2>&1`],
+ flags: Gio.SubprocessFlags.NONE,
+ });
</code_context>
<issue_to_address>
**issue (bug_risk):** Quote the log file path with GLib.shell_quote instead of embedding it directly in the shell redirection string.
`logFile` is built from `get_tmp_dir()` and the workspace basename, so it can still contain characters (e.g. quotes) that break the current `> "${logFile}" 2>&1` quoting. To harden this, build the command as `... "-c", `${shellCmd} > ${GLib.shell_quote(logFile)} 2>&1`` so the redirection target is shell-escaped regardless of the folder name.
</issue_to_address>
### Comment 2
<location path="src/docker.js" line_range="399-403" />
<code_context>
+ * captured log on failure.
+ * @param {String} localFolder The workspace folder path for the devcontainer
+ */
+export const runDevcontainerUp = (localFolder) => {
+ const folderName = GLib.path_get_basename(localFolder);
+ Main.notify("Devcontainer", `Starting ${folderName}…`);
+
+ runDevcontainerProcess(
+ `devcontainer up --workspace-folder "${localFolder}"`,
+ localFolder
</code_context>
<issue_to_address>
**issue (bug_risk):** Use shell-escaping for `localFolder` in the devcontainer CLI commands to handle paths with quotes or special characters safely.
Both `runDevcontainerUp` and `runDevcontainerRecreate` interpolate `localFolder` into the command as `--workspace-folder "${localFolder}"`. This covers spaces but breaks if the path itself contains a double quote. Since `localFolder` comes from labels/filesystem paths, use `GLib.shell_quote(localFolder)` instead of manual quoting, e.g.:
```js
const quotedFolder = GLib.shell_quote(localFolder);
runDevcontainerProcess(
`devcontainer up --workspace-folder ${quotedFolder}`,
localFolder,
);
```
Apply the same pattern to the `--remove-existing-container` variant.
</issue_to_address>
### Comment 3
<location path="CHANGELOG.md" line_range="14" />
<code_context>
+
## [32] - 2026-04-27
Added Gnome 50 support
</code_context>
<issue_to_address>
**nitpick (typo):** Consider using the canonical capitalization "GNOME" instead of "Gnome".
Using "GNOME" here matches the project's official capitalization and aligns with upstream naming.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+337
to
+346
| const runDevcontainerProcess = (shellCmd, localFolder) => { | ||
| const folderName = GLib.path_get_basename(localFolder); | ||
| // Include a timestamp to avoid collisions across concurrent runs or | ||
| // multiple workspaces that share the same basename. | ||
| const logFile = `${GLib.get_tmp_dir()}/devcontainer-${folderName}-${Date.now()}.log`; | ||
| const userShell = getUserShell(); | ||
| const shellFlags = getLoginShellFlags(); | ||
|
|
||
| return new Promise((resolve, reject) => { | ||
| try { |
There was a problem hiding this comment.
issue (bug_risk): Quote the log file path with GLib.shell_quote instead of embedding it directly in the shell redirection string.
logFile is built from get_tmp_dir() and the workspace basename, so it can still contain characters (e.g. quotes) that break the current > "${logFile}" 2>&1 quoting. To harden this, build the command as ... "-c", ${shellCmd} > ${GLib.shell_quote(logFile)} 2>&1`` so the redirection target is shell-escaped regardless of the folder name.
Comment on lines
+399
to
+403
| export const runDevcontainerUp = (localFolder) => { | ||
| const folderName = GLib.path_get_basename(localFolder); | ||
| Main.notify("Devcontainer", `Starting ${folderName}…`); | ||
|
|
||
| runDevcontainerProcess( |
There was a problem hiding this comment.
issue (bug_risk): Use shell-escaping for localFolder in the devcontainer CLI commands to handle paths with quotes or special characters safely.
Both runDevcontainerUp and runDevcontainerRecreate interpolate localFolder into the command as --workspace-folder "${localFolder}". This covers spaces but breaks if the path itself contains a double quote. Since localFolder comes from labels/filesystem paths, use GLib.shell_quote(localFolder) instead of manual quoting, e.g.:
const quotedFolder = GLib.shell_quote(localFolder);
runDevcontainerProcess(
`devcontainer up --workspace-folder ${quotedFolder}`,
localFolder,
);Apply the same pattern to the --remove-existing-container variant.
|
|
||
| ## [32] - 2026-04-27 | ||
|
|
||
| Added Gnome 50 support |
There was a problem hiding this comment.
nitpick (typo): Consider using the canonical capitalization "GNOME" instead of "Gnome".
Using "GNOME" here matches the project's official capitalization and aligns with upstream naming.
kiuma
force-pushed
the
feature/devcontainers
branch
from
f66d741 to
da4c792
Compare
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 by Sourcery
Add devcontainer awareness and CLI integration to the GNOME Shell Docker extension while deferring heavy checks to avoid blocking shell startup and updating schemas/build packaging.
New Features:
Bug Fixes:
Enhancements:
Build:
Documentation:
Chores: