Skip to content

feat: add TERMINAL_TOOL_TIMEOUT configuration#261

Open
mason5052 wants to merge 2 commits intovxcontrol:mainfrom
mason5052:codex/issue-256-terminal-timeout
Open

feat: add TERMINAL_TOOL_TIMEOUT configuration#261
mason5052 wants to merge 2 commits intovxcontrol:mainfrom
mason5052:codex/issue-256-terminal-timeout

Conversation

@mason5052
Copy link
Copy Markdown
Contributor

@mason5052 mason5052 commented Apr 15, 2026

Summary

  • add TERMINAL_TOOL_TIMEOUT as a configurable environment variable with Docker Compose, config, installer, and docs support
  • use the configured timeout as the terminal tool default when a tool call sets timeout=0
  • add unit coverage for config parsing and terminal timeout normalization

Problem

Issue #256 reports that long-running terminal commands can time out with no operator-side configuration to change the default behavior.

At the moment, terminal execution falls back to a hardcoded default timeout in the backend. Users running PentAGI through Docker Compose can adjust HTTP_CLIENT_TIMEOUT, but there is no equivalent setting for terminal tool execution.

Solution

Add a new TERMINAL_TOOL_TIMEOUT setting and thread it through the relevant execution paths.

  • backend/pkg/config parses TERMINAL_TOOL_TIMEOUT with a default of 600
  • docker-compose.yml and .env.example expose the variable for Compose-based deployments
  • terminal tool constructors now receive the configured default timeout and apply it when a tool call uses timeout=0
  • 0 is supported as "no server-side default timeout"
  • installer server settings now expose the same variable
  • docs and tool descriptions were updated so the behavior is visible to users and agents

This keeps explicit tool timeouts intact while giving operators a configuration-level fallback for long-running commands.

User Impact

Operators can now increase the default terminal execution window for long-running commands without patching the codebase. Compose users can set TERMINAL_TOOL_TIMEOUT in .env, and installer users can change the same setting from the server settings flow.

Closes #256.

Test Plan

  • go test ./pkg/config ./pkg/tools/...
  • go test ./cmd/ftester/... ./cmd/installer/wizard/controller ./cmd/installer/wizard/models ./cmd/installer/wizard/locale
  • gofmt -w on changed Go files
  • git diff --check

Notes

go test ./cmd/installer/wizard/... still hits pre-existing Windows host issues in cmd/installer/wizard/terminal because that package expects POSIX commands such as echo, sh, and cat to be available in PATH. This PR does not modify that package.

Signed-off-by: Mason Kim(ZINUS US_SALES) <mkim@zinus.com>
@mason5052 mason5052 marked this pull request as ready for review April 15, 2026 16:38
Copilot AI review requested due to automatic review settings April 15, 2026 16:38
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds operator-configurable default terminal execution timeout via TERMINAL_TOOL_TIMEOUT, threading it through backend config, tool construction, installer wizard, Docker Compose env wiring, and documentation, with unit tests covering parsing and timeout normalization.

Changes:

  • Introduce TERMINAL_TOOL_TIMEOUT config/env var (default 600, 0 disables server-side default) and expose it in Docker Compose + installer wizard.
  • Pass configured timeout into terminal tool constructors and apply it when tool calls request timeout=0, plus add normalization logic/tests.
  • Update docs/tool descriptions/README/.env.example to document the new behavior.

Reviewed changes

Copilot reviewed 16 out of 16 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
docker-compose.yml Exposes TERMINAL_TOOL_TIMEOUT into the container environment.
backend/pkg/config/config.go Adds TerminalToolTimeout to backend env-parsed config.
backend/pkg/config/config_test.go Adds unit coverage for TERMINAL_TOOL_TIMEOUT parsing defaults/overrides.
backend/pkg/tools/tools.go Threads configured timeout into terminal tool construction for multiple executors.
backend/pkg/tools/terminal.go Implements configurable default timeout + normalization and supports “no timeout” mode.
backend/pkg/tools/terminal_test.go Adds unit coverage for timeout normalization behavior.
backend/pkg/tools/registry.go Updates terminal tool description to mention timeout=0 behavior.
backend/pkg/tools/args.go Updates JSON schema description for timeout semantics.
backend/docs/config.md Documents TERMINAL_TOOL_TIMEOUT and shows constructor usage.
backend/cmd/installer/wizard/controller/controller.go Adds TERMINAL_TOOL_TIMEOUT to installer-managed server settings with defaults.
backend/cmd/installer/wizard/models/server_settings_form.go Adds installer UI field + save handling for terminal timeout.
backend/cmd/installer/wizard/locale/locale.go Adds installer locale strings/help text + env var description.
backend/cmd/ftester/worker/executor.go Passes configured terminal timeout into tool creation in ftester.
backend/cmd/ftester/mocks/tools.go Updates mocked tool description to mention TERMINAL_TOOL_TIMEOUT.
README.md Documents TERMINAL_TOOL_TIMEOUT in environment variable section.
.env.example Adds example/commentary for TERMINAL_TOOL_TIMEOUT.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +292 to +295
sections = append(sections, fmt.Sprintf("• %s: %s", locale.ServerSettingsTerminalToolTimeoutHint, terminalTimeout))
} else if terminalTimeout := cfg.TerminalToolTimeout.Default; terminalTimeout != "" {
terminalTimeout = m.GetStyles().Muted.Render(terminalTimeout + "s")
sections = append(sections, fmt.Sprintf("• %s: %s", locale.ServerSettingsTerminalToolTimeoutHint, terminalTimeout))
Copy link

Copilot AI Apr 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The bullet prefix here is garbled ("•") and will render incorrectly in the installer UI. Use the same "•" character used throughout this method for list items.

Suggested change
sections = append(sections, fmt.Sprintf("• %s: %s", locale.ServerSettingsTerminalToolTimeoutHint, terminalTimeout))
} else if terminalTimeout := cfg.TerminalToolTimeout.Default; terminalTimeout != "" {
terminalTimeout = m.GetStyles().Muted.Render(terminalTimeout + "s")
sections = append(sections, fmt.Sprintf("• %s: %s", locale.ServerSettingsTerminalToolTimeoutHint, terminalTimeout))
sections = append(sections, fmt.Sprintf(" %s: %s", locale.ServerSettingsTerminalToolTimeoutHint, terminalTimeout))
} else if terminalTimeout := cfg.TerminalToolTimeout.Default; terminalTimeout != "" {
terminalTimeout = m.GetStyles().Muted.Render(terminalTimeout + "s")
sections = append(sections, fmt.Sprintf(" %s: %s", locale.ServerSettingsTerminalToolTimeoutHint, terminalTimeout))

Copilot uses AI. Check for mistakes.
Comment thread backend/pkg/tools/terminal.go Outdated
case timeout > 0 && timeout <= maxRuntimeExecCommandTimeout:
return timeout
case timeout > maxRuntimeExecCommandTimeout:
return t.configuredExecTimeout()
Copy link

Copilot AI Apr 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

normalizeExecTimeout currently treats any requested timeout > maxRuntimeExecCommandTimeout as "use configured default". If TERMINAL_TOOL_TIMEOUT is configured as 0 (no default), an explicitly-too-large timeout will end up disabling timeouts entirely, bypassing the intended max. Consider clamping oversized explicit values to maxRuntimeExecCommandTimeout (or returning an error) instead of falling back to the configured default when the request is out of range.

Suggested change
return t.configuredExecTimeout()
return maxRuntimeExecCommandTimeout

Copilot uses AI. Check for mistakes.
DockerWorkDir string `env:"DOCKER_WORK_DIR"`
DockerDefaultImage string `env:"DOCKER_DEFAULT_IMAGE" envDefault:"debian:latest"`
DockerDefaultImageForPentest string `env:"DOCKER_DEFAULT_IMAGE_FOR_PENTEST" envDefault:"vxcontrol/kali-linux"`
TerminalToolTimeout int `env:"TERMINAL_TOOL_TIMEOUT" envDefault:"600"`
Copy link

Copilot AI Apr 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TERMINAL_TOOL_TIMEOUT is parsed as an int with no validation in NewConfig(). A negative value is currently accepted by env parsing and later interpreted as 0 (no timeout) by the terminal tool, which is undocumented and potentially unsafe. Consider validating this in NewConfig (e.g., reject values < 0) to match the installer validation behavior.

Suggested change
TerminalToolTimeout int `env:"TERMINAL_TOOL_TIMEOUT" envDefault:"600"`
TerminalToolTimeout uint `env:"TERMINAL_TOOL_TIMEOUT" envDefault:"600"`

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Feature request: Expose TERMINAL_TOOL_TIMEOUT as configurable environment variable

3 participants