sandboxing for shell command execution#128
Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces sandboxing functionality for shell command execution by running commands inside Docker containers instead of directly on the host system. This provides isolation and security for executing potentially dangerous commands.
Key Changes:
- Implements a new Docker-based container execution module with streaming output support
- Adds an abstract ExecutorInterface to allow switching between host and container execution
- Introduces sandbox mode (enabled by default) controllable via INFRAGPT_ISOLATED environment variable
Reviewed changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 20 comments.
Show a summary per file
| File | Description |
|---|---|
cli/src/infragpt/container.py |
New module implementing Docker container-based command execution with ExecutorInterface, ContainerRunner class, and helper functions for Docker availability checks and cleanup |
cli/src/infragpt/shell.py |
Modified CommandExecutor to implement ExecutorInterface and added cleanup method for consistency |
cli/src/infragpt/tools.py |
Added executor factory pattern to switch between host and container executors based on sandbox mode, with module-level singleton management |
cli/src/infragpt/main.py |
Added sandbox initialization at startup with Docker availability checks, container cleanup in finally block, and error handling for Docker issues |
cli/src/infragpt/images/sandbox/Dockerfile |
New Dockerfile defining the sandbox container image with common utilities, Google Cloud SDK, and kubectl |
cli/src/infragpt/images/sandbox/Makefile |
Build automation for creating multi-architecture sandbox images and pushing to GitHub Container Registry |
cli/pyproject.toml |
Added docker>=7.1.0 dependency for container management |
cli/CLAUDE.md |
New file documenting development guidelines (use uv, minimal comments) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Caution Review failedThe pull request is closed. Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds a Docker-backed sandbox executor and image, integrates sandbox-aware executor selection and lifecycle into CLI startup and shutdown, updates executor interfaces, small docs and dependency changes, and simplifies several internal agent/history/LLM adapter flows. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI (main)
participant Tools as tools.get_executor
participant Container as ContainerRunner
participant Docker as Docker daemon
CLI->>Tools: startup -> is_sandbox_mode?
Tools->>Container: cleanup_old_containers()
Tools->>Container: get_executor() / start()
Container->>Docker: pull image / create & start container
Docker-->>Container: container running
Note over CLI,Tools: User issues shell command
Tools->>Container: execute_command(command)
Container->>Docker: docker exec -> run command
Docker-->>Container: stream stdout/stderr
Container-->>Tools: return (exit_code, output, was_cancelled)
Note over CLI: Shutdown
CLI->>Tools: cleanup_executor()
Tools->>Container: stop & remove container
Container->>Docker: remove container / close client
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
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: 2
♻️ Duplicate comments (4)
cli/src/infragpt/images/sandbox/Dockerfile (1)
40-44: Verify kubectl binary integrity with checksum.The kubectl binary is downloaded without checksum verification, which poses a supply-chain security risk if the download is tampered with.
# Install kubectl ARG TARGETARCH -RUN curl -LO "https://dl.k8s.io/release/$(curl -L -s https://dl.k8s.io/release/stable.txt)/bin/linux/${TARGETARCH}/kubectl" \ +RUN KUBECTL_VERSION="$(curl -L -s https://dl.k8s.io/release/stable.txt)" \ + && curl -LO "https://dl.k8s.io/release/${KUBECTL_VERSION}/bin/linux/${TARGETARCH}/kubectl" \ + && curl -LO "https://dl.k8s.io/release/${KUBECTL_VERSION}/bin/linux/${TARGETARCH}/kubectl.sha256" \ + && echo "$(cat kubectl.sha256) kubectl" | sha256sum --check \ && install -o root -g root -m 0755 kubectl /usr/local/bin/kubectl \ - && rm kubectl + && rm kubectl kubectl.sha256cli/src/infragpt/main.py (1)
148-163: Potential resource leak ifexecutor.start()fails.If
get_executor()succeeds butexecutor.start()raises an exception other thanDockerNotAvailableError, the executor object is created but never cleaned up. Consider wrapping the start in a try block that cleans up on failure.try: removed = cleanup_old_containers() if removed > 0: console.print(f"[dim]Cleaned up {removed} old sandbox container(s)[/dim]") console.print( "[yellow]Sandbox mode enabled - starting Docker container...[/yellow]" ) executor = get_executor() - executor.start() # This checks Docker availability + try: + executor.start() # This checks Docker availability + except Exception: + executor.cleanup() + raise console.print("[green]Sandbox container ready.[/green]\n") except DockerNotAvailableError as e:cli/src/infragpt/container.py (2)
183-192:remove=Truemay cause issues instop()method.With
remove=True, the container is automatically removed when stopped. In thestop()method (lines 290-299), ifcontainer.stop()succeeds but subsequent code tries to interact with the container, or if the container was already auto-removed, operations will fail silently.This is mostly benign since you set
self.container = Noneafter stop, but be aware thatcontainer.kill()in the fallback (line 296) may fail if auto-removal already occurred.
244-253:pkill -P 1may not effectively cancel the running command.In Docker containers, PID 1 is typically the container's init process (
tail -f /dev/nullin this case), not the parent of exec'd commands. Thepkill -P 1approach won't reliably kill the exec'd process since exec'd commands have their own PID hierarchy.A more reliable approach would be to track the exec's PID:
try: exec_info = self.client.api.exec_inspect(exec_id) pid = exec_info.get("Pid") if pid and pid > 0: self.client.api.exec_start( self.client.api.exec_create( container=self.container.id, cmd=["/bin/sh", "-c", f"kill -9 {pid} 2>/dev/null"], ) ) except Exception: pass # Best effort cancellation
🧹 Nitpick comments (4)
cli/src/infragpt/images/sandbox/Makefile (1)
4-4: Consider adding acleantarget.A
cleantarget would be helpful for removing local images and resetting the buildx builder during development.-.PHONY: build build-amd64 build-arm64 push push-amd64 push-arm64 all login setup +.PHONY: build build-amd64 build-arm64 push push-amd64 push-arm64 all login setup clean +clean: + docker rmi $(LOCAL_IMAGE) 2>/dev/null || true + docker rmi $(IMAGE_NAME):latest-amd64 2>/dev/null || true + docker rmi $(IMAGE_NAME):latest-arm64 2>/dev/null || truecli/src/infragpt/images/sandbox/Dockerfile (1)
21-21: Consider whethersudois necessary in this sandbox container.Installing
sudoincreases the attack surface. If the container always runs as root and doesn't need privilege escalation, consider removing it.cli/src/infragpt/tools.py (1)
40-51: Late import creates potential for double cleanup.The late import of
cleanup_executor as cleanup_containerfrom.containeris fine for avoiding circular imports, but note thatmain.pyalso importscleanup_executordirectly fromcontainerand calls both. This could result in double cleanup whenmain.pycalls bothcleanup_executor()andcleanup_tools_executor().Consider documenting that callers should only call one cleanup function, or make the cleanup idempotent by checking if already cleaned.
cli/src/infragpt/container.py (1)
70-77: Add exception chaining for better traceability.When re-raising as a custom exception, chain it with
from eto preserve the original traceback for debugging.def ensure_docker_available() -> None: """Ensure Docker daemon is available and running. Raises DockerNotAvailableError if not.""" try: client = docker.from_env() client.ping() client.close() except docker.errors.DockerException as e: - raise DockerNotAvailableError(f"Docker error: {e}") + raise DockerNotAvailableError(f"Docker error: {e}") from e
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
cli/CLAUDE.md(1 hunks)cli/pyproject.toml(2 hunks)cli/src/infragpt/container.py(1 hunks)cli/src/infragpt/images/sandbox/Dockerfile(1 hunks)cli/src/infragpt/images/sandbox/Makefile(1 hunks)cli/src/infragpt/main.py(3 hunks)cli/src/infragpt/shell.py(2 hunks)cli/src/infragpt/tools.py(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
cli/src/infragpt/tools.py (2)
cli/src/infragpt/container.py (6)
ExecutorInterface(39-58)is_sandbox_mode(61-67)get_executor(113-118)cleanup_executor(121-126)cleanup(56-58)cleanup(305-307)cli/src/infragpt/shell.py (2)
CommandExecutor(31-259)cleanup(257-259)
cli/src/infragpt/container.py (1)
cli/src/infragpt/tools.py (2)
get_executor(26-37)cleanup_executor(40-51)
cli/src/infragpt/shell.py (1)
cli/src/infragpt/container.py (3)
ExecutorInterface(39-58)cleanup(56-58)cleanup(305-307)
🪛 checkmake (0.2.2)
cli/src/infragpt/images/sandbox/Makefile
[warning] 4-4: Missing required phony target "clean"
(minphony)
[warning] 4-4: Missing required phony target "test"
(minphony)
🪛 Ruff (0.14.8)
cli/src/infragpt/container.py
77-77: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
77-77: Avoid specifying long messages outside the exception class
(TRY003)
98-99: try-except-pass detected, consider logging the exception
(S110)
98-98: Do not catch blind exception: Exception
(BLE001)
102-103: try-except-pass detected, consider logging the exception
(S110)
102-102: Do not catch blind exception: Exception
(BLE001)
105-105: Consider moving this statement to an else block
(TRY300)
106-106: Do not catch blind exception: Exception
(BLE001)
173-173: Do not catch blind exception: Exception
(BLE001)
174-177: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
174-177: Avoid specifying long messages outside the exception class
(TRY003)
205-205: Avoid specifying long messages outside the exception class
(TRY003)
252-253: try-except-pass detected, consider logging the exception
(S110)
252-252: Do not catch blind exception: Exception
(BLE001)
263-263: Consider moving this statement to an else block
(TRY300)
265-265: Do not catch blind exception: Exception
(BLE001)
285-286: try-except-pass detected, consider logging the exception
(S110)
285-285: Do not catch blind exception: Exception
(BLE001)
294-294: Do not catch blind exception: Exception
(BLE001)
297-298: try-except-pass detected, consider logging the exception
(S110)
297-297: Do not catch blind exception: Exception
(BLE001)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (go)
🔇 Additional comments (17)
cli/CLAUDE.md (1)
1-2: LGTM!Simple and clear AI guidance instructions.
cli/src/infragpt/shell.py (2)
25-31: LGTM!The
CommandExecutorcorrectly implementsExecutorInterface, enabling polymorphic executor selection between host and container-based execution. The import and inheritance are properly structured.
257-259: LGTM!The no-op
cleanup()implementation is appropriate for the host executor since there are no container resources to manage.cli/src/infragpt/main.py (1)
173-179: Good use of try/finally for cleanup.The cleanup logic ensures sandbox resources are released even if an exception occurs during agent execution.
cli/src/infragpt/images/sandbox/Makefile (1)
1-37: Well-structured multi-architecture build configuration.The Makefile properly handles buildx setup, multi-arch builds, and push operations. The
--provenance=falseflag avoids attestation compatibility issues.cli/src/infragpt/images/sandbox/Dockerfile (1)
1-1: LGTM!Good choice to pin Ubuntu to a specific version (24.04) for reproducible builds.
cli/src/infragpt/tools.py (2)
22-37: LGTM!Good implementation of the sandbox-aware executor selection with lazy initialization for the host executor singleton.
185-192: LGTM!The command execution correctly uses
get_executor()to obtain the appropriate executor based on sandbox mode, maintaining consistent behavior for both host and container execution paths.cli/pyproject.toml (1)
30-30: Docker dependency appropriately added for container-based sandbox execution.The
docker>=7.1.0dependency correctly enables the Docker SDK functionality used incontainer.pyfor sandbox command execution. The code uses standard Docker API calls (docker.from_env(),client.containers.list(),container.stop()) that are fully compatible with version 7.1.0 and later. The version constraint allows for appropriate patch and minor updates.cli/src/infragpt/container.py (8)
23-30: LGTM!Architecture detection logic is sound, correctly mapping ARM variants and defaulting to amd64.
39-58: LGTM!Clean abstract interface definition with appropriate method signatures and documentation.
61-67: LGTM!Opt-out design for sandbox mode is sensible for security-by-default behavior.
84-110: LGTM with minor note.Resource cleanup with the
finallyblock is correctly implemented. Silent exception handling during container cleanup is acceptable for best-effort cleanup semantics.
113-126: LGTM!Singleton pattern is appropriate for CLI context where single-threaded execution is expected.
212-220: LGTM - Shell injection mitigated and timeout implemented.Good use of
shlex.quote()onself.current_cwdto prevent shell injection, and thetimeoutcommand prefix correctly applies the configured timeout.
269-286: LGTM!The marker-based approach for extracting the working directory is a good improvement over re-executing commands. Silent failure handling is acceptable since cwd tracking is a convenience feature that shouldn't break command execution.
288-307: LGTM!The stop-then-kill fallback pattern is appropriate for reliable container cleanup. The
cleanup()alias correctly implements theExecutorInterface.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
cli/src/infragpt/main.py (1)
145-163: Consider adding cleanup before early exit on Docker failure.If
executor.start()fails partway through (after the container is created but before completion), the container may remain until the next CLI invocation whencleanup_old_containers()runs. While this is mitigated by the startup cleanup andremove=Trueon the container, adding explicit cleanup before the early exit would be more robust.except DockerNotAvailableError as e: console.print(f"[red]Error: {e}[/red]") console.print( "Please fix the issue above or disable sandbox mode with INFRAGPT_ISOLATED=false" ) + cleanup_executor() sys.exit(1)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cli/src/infragpt/main.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
cli/src/infragpt/main.py (4)
cli/src/infragpt/container.py (6)
is_sandbox_mode(61-67)get_executor(113-118)cleanup_old_containers(84-110)DockerNotAvailableError(33-36)cleanup_executor(121-126)start(161-192)cli/src/infragpt/tools.py (2)
get_executor(26-37)cleanup_executor(40-51)services/agent/src/grpc/server.py (1)
start(26-53)cli/src/infragpt/agent.py (1)
run_shell_agent(324-327)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (go)
🔇 Additional comments (1)
cli/src/infragpt/main.py (1)
15-16: Imports are correct and past redundancy concern is addressed.The imports are now properly structured -
cleanup_executoris imported only frominfragpt.tools, which correctly delegates tocontainer.cleanup_executor()in sandbox mode per the code intools.py(Lines 44-48).
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
cli/src/infragpt/container.py (3)
70-77: Consider exception chaining for better debugging.The function works correctly, but could benefit from explicit exception chaining:
- except docker.errors.DockerException as e: - raise DockerNotAvailableError(f"Docker error: {e}") + except docker.errors.DockerException as e: + raise DockerNotAvailableError(f"Docker error: {e}") from eThis preserves the full stack trace for debugging.
84-110: Consider logging cleanup failures for debugging.The function properly uses a
finallyblock to close the Docker client (addressing a past review comment). However, silent exception handling at lines 98-99 and 102-103 could make debugging difficult.try: container.stop(timeout=5) - except Exception: - pass + except Exception as e: + if console: # Import console at module level + console.print(f"[dim]Warning: Failed to stop container: {e}[/dim]") try: container.remove(force=True) - except Exception: - pass + except Exception as e: + if console: + console.print(f"[dim]Warning: Failed to remove container: {e}[/dim]")This helps diagnose issues during cleanup without breaking the flow.
161-190: Container setup is correct. Consider security implications of rw mount.The container setup properly handles image pulling and volume mounting. The read-write mount at line 178 is intentional for sandbox functionality but does allow container to modify host files. This is a documented trade-off.
For production use, consider:
- Adding a flag to restrict mounts to read-only when write access isn't needed
- Documenting which operations require write access
- Considering user namespacing to limit damage from container escape
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
cli/src/infragpt/agent.py(2 hunks)cli/src/infragpt/container.py(1 hunks)cli/src/infragpt/history.py(2 hunks)cli/src/infragpt/llm_adapter.py(2 hunks)cli/src/infragpt/shell.py(6 hunks)cli/src/infragpt/tools.py(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
cli/src/infragpt/container.py (2)
cli/src/infragpt/shell.py (2)
execute_command(47-121)cleanup(223-225)cli/src/infragpt/tools.py (2)
get_executor(25-34)cleanup_executor(37-46)
cli/src/infragpt/tools.py (2)
cli/src/infragpt/container.py (6)
ExecutorInterface(39-58)is_sandbox_mode(61-67)get_executor(113-118)cleanup_executor(121-126)cleanup(56-58)cleanup(288-290)cli/src/infragpt/shell.py (2)
CommandExecutor(30-225)cleanup(223-225)
🪛 GitHub Check: CodeQL
cli/src/infragpt/history.py
[failure] 99-99: Clear-text storage of sensitive information
This expression stores sensitive data (password) as clear text.
This expression stores sensitive data (password) as clear text.
This expression stores sensitive data (password) as clear text.
This expression stores sensitive data (password) as clear text.
This expression stores sensitive data (password) as clear text.
This expression stores sensitive data (password) as clear text.
This expression stores sensitive data (password) as clear text.
This expression stores sensitive data (password) as clear text.
This expression stores sensitive data (password) as clear text.
This expression stores sensitive data (password) as clear text.
This expression stores sensitive data (password) as clear text.
🪛 Ruff (0.14.8)
cli/src/infragpt/container.py
77-77: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
77-77: Avoid specifying long messages outside the exception class
(TRY003)
98-99: try-except-pass detected, consider logging the exception
(S110)
98-98: Do not catch blind exception: Exception
(BLE001)
102-103: try-except-pass detected, consider logging the exception
(S110)
102-102: Do not catch blind exception: Exception
(BLE001)
105-105: Consider moving this statement to an else block
(TRY300)
106-106: Do not catch blind exception: Exception
(BLE001)
172-172: Do not catch blind exception: Exception
(BLE001)
173-176: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
173-176: Avoid specifying long messages outside the exception class
(TRY003)
203-203: Avoid specifying long messages outside the exception class
(TRY003)
244-245: try-except-pass detected, consider logging the exception
(S110)
244-244: Do not catch blind exception: Exception
(BLE001)
253-253: Consider moving this statement to an else block
(TRY300)
255-255: Do not catch blind exception: Exception
(BLE001)
268-269: try-except-pass detected, consider logging the exception
(S110)
268-268: Do not catch blind exception: Exception
(BLE001)
277-277: Do not catch blind exception: Exception
(BLE001)
280-281: try-except-pass detected, consider logging the exception
(S110)
280-280: Do not catch blind exception: Exception
(BLE001)
cli/src/infragpt/tools.py
157-157: Avoid specifying long messages outside the exception class
(TRY003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (go)
🔇 Additional comments (12)
cli/src/infragpt/shell.py (1)
25-25: LGTM! Clean ExecutorInterface implementation.The
CommandExecutornow properly implementsExecutorInterfacewith the requiredexecute_commandandcleanupmethods. The no-opcleanup()is appropriate for the host executor since it doesn't manage external resources like containers.Also applies to: 30-30, 223-225
cli/src/infragpt/tools.py (2)
22-34: LGTM! Sandbox-aware executor selection is clean.The module-level singleton pattern for
_host_executorand theget_executor()function properly route execution based on sandbox mode. The lazy initialization is appropriate for CLI usage.
37-46: LGTM! Cleanup logic is symmetric with executor selection.The
cleanup_executor()properly delegates to the appropriate executor and handles module-level state correctly. The dynamic import at line 42 is a reasonable approach to avoid circular dependencies.cli/src/infragpt/history.py (1)
73-105: Sanitization logic is sound, but be aware of user input risks.The CodeQL warning at line 99 is partially a false positive. The code properly sanitizes data and uses an allowlist approach. However, there's an inherent limitation: if users type sensitive information directly in their prompts (e.g., "set my password to secret123"), it will be logged since
user_inputandassistant_responseare in the allowlist.This is a known trade-off for conversation logging. Consider documenting this behavior in user-facing documentation.
cli/src/infragpt/llm_adapter.py (1)
90-90: LGTM! Minor formatting adjustments.The changes are cosmetic and maintain correct behavior. Setting
content: Noneat line 171 is the proper OpenAI format for tool call messages.Also applies to: 171-171
cli/src/infragpt/agent.py (1)
113-113: Good security awareness in the comment.The warning about FileHistory writing clear-text inputs to disk is important and well-documented. This aligns with the security considerations in history.py.
cli/src/infragpt/container.py (6)
23-30: LGTM! Platform detection is appropriate.The platform detection correctly handles ARM64 variants and defaults to AMD64. This covers the most common architectures for this use case.
132-159: LGTM! Configuration setup is clean.The
__init__method properly initializes all configuration. Note thattimeout(line 154) is indeed used inexecute_commandat line 212 via thetimeoutcommand prefix, so the past review concern about unused timeout is no longer valid.
210-216: Command construction properly handles shell injection.The use of
shlex.quote()forself.current_cwdat line 214 correctly prevents shell injection for the directory path. The user command is intentionally not quoted to allow shell interpretation.
259-269: LGTM! Working directory tracking is efficient and safe.The implementation properly extracts the working directory from command output using a marker approach. This is much better than re-executing commands (as noted in past reviews) and correctly validates the path before updating.
271-290: LGTM! Container cleanup is defensive and thorough.The
stop()method properly handles cleanup with fallback tokill()if graceful stop fails. The defensive exception handling is appropriate for cleanup code where we want to ensure resources are released even if some steps fail.The container is created with
remove=True(line 189), so it auto-removes when stopped, but the exception handling ensures this method is safe to call even if the container is already gone.
80-81: LGTM! Singleton pattern is correctly implemented with proper cleanup.The module-level singleton pattern for
_executoris appropriately used for single-threaded CLI execution. The implementation is safe:cleanup_executor()is called in a finally block in the main execution flow (main.py:174), ensuring resources are always cleaned up when the application exits. This approach is consistent with the pattern used intools.pyfor_host_executor.
| except KeyboardInterrupt: | ||
| self.cancelled = True | ||
| console.print("\n[yellow]Command cancelled by user[/yellow]") | ||
| try: | ||
| self.client.api.exec_start( | ||
| self.client.api.exec_create( | ||
| container=self.container.id, | ||
| cmd=["/bin/sh", "-c", "pkill -P 1"], | ||
| ) | ||
| ) | ||
| except Exception: | ||
| pass |
There was a problem hiding this comment.
Cancellation logic may not reliably kill the target process.
The pkill -P 1 command at line 241 attempts to kill all child processes of PID 1 (init), which may not effectively target the specific command being executed. This was noted in a past review and remains unaddressed.
The current approach has limitations:
- May kill unrelated processes in the container
- May fail to kill the target if it's not a direct child of PID 1
- No guarantee the exec'd command tree is terminated
Consider tracking the exec PID and using a more targeted kill:
except KeyboardInterrupt:
self.cancelled = True
console.print("\n[yellow]Command cancelled by user[/yellow]")
try:
# Get the PID of the exec process
exec_info = self.client.api.exec_inspect(exec_id)
pid = exec_info.get("Pid")
if pid and pid > 0:
# Kill the specific process and its children
kill_cmd = f"kill -TERM -{pid} 2>/dev/null || kill -9 -{pid}"
self.client.api.exec_start(
self.client.api.exec_create(
container=self.container.id,
cmd=["/bin/sh", "-c", kill_cmd],
)
)
except Exception:
pass # Ignore errors during cancellationNote: The negative PID (-pid) sends the signal to the entire process group.
🧰 Tools
🪛 Ruff (0.14.8)
244-245: try-except-pass detected, consider logging the exception
(S110)
244-244: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
In cli/src/infragpt/container.py around lines 234 to 245, the cancellation block
uses `pkill -P 1` which can kill unrelated processes and may not target the
exec'd command; instead, when handling KeyboardInterrupt set self.cancelled and
print the cancellation message, capture the exec id when starting the exec, call
the API to inspect that exec to retrieve its PID, and if a valid PID is returned
use a targeted kill of the exec process group (send TERM to -PID, fallback to
KILL) by exec'ing a shell command in the container; wrap the inspect/kill in a
try/except and ignore errors during cancellation so cancellation remains
best-effort without affecting other container processes.
Summary by CodeRabbit
New Features
Documentation
Chores
Bug Fixes / UX
✏️ Tip: You can customize this high-level summary in your review settings.