TUI: Lack of input validation for timeout_seconds in shell_exec.py #8896

Open
opened 2026-04-14 03:35:19 +00:00 by HAL9000 · 2 comments
Owner

Metadata

  • Commit Message: fix(tui): add input validation for timeout_seconds in run_shell_command
  • Branch: fix/tui-shell-exec-timeout-validation

Background and Context

The run_shell_command function in src/cleveragents/tui/input/shell_exec.py accepts a timeout_seconds parameter that is passed directly to subprocess.run without any validation. According to the project's Argument Validation guidelines, all public and protected class methods must validate arguments as the first guard — including value range checks to ensure numeric values are within acceptable bounds.

A caller supplying a zero or negative value for timeout_seconds would cause subprocess.run to behave unexpectedly:

  • A value of 0 causes an immediate TimeoutExpired on virtually every command.
  • A negative value results in undefined/implementation-dependent behavior in the Python standard library.

This violates the fail-fast principle: invalid inputs should be rejected immediately at the boundary where they enter, not allowed to propagate silently into subprocess.run.

Code Evidence:

In src/cleveragents/tui/input/shell_exec.py, the run_shell_command function (around lines 34–39) accepts timeout_seconds: int = 30 and passes it directly to subprocess.run(... timeout=timeout_seconds ...) with no guard against non-positive values.

Expected Behavior

run_shell_command should validate timeout_seconds at the top of the function body, before any other logic. If the value is less than or equal to zero, the function should raise a ValueError with a clear message (e.g., "timeout_seconds must be a positive integer, got: {timeout_seconds}"). This ensures fail-fast behavior and surfaces misconfiguration immediately at the call site.

Acceptance Criteria

  • run_shell_command raises ValueError when timeout_seconds <= 0, with a descriptive error message.
  • The validation occurs before any other logic in the function body (fail-fast).
  • The default value of 30 continues to work correctly.
  • All existing tests continue to pass.
  • New BDD scenarios cover: timeout_seconds = 0, timeout_seconds = -1, and a valid positive value.
  • Test coverage remains at or above the project-defined threshold (≥ 97%).
  • All nox sessions pass with no errors.

Subtasks

  • Add ValueError guard for timeout_seconds <= 0 at the top of run_shell_command in src/cleveragents/tui/input/shell_exec.py
  • Write BDD scenarios (Behave/Gherkin) covering zero, negative, and valid timeout_seconds values
  • Implement step definitions for the new scenarios
  • Tests (Robot): Add integration test verifying the validation boundary
  • Verify coverage ≥ 97% via nox -s coverage_report
  • Run nox (all default sessions) and fix any errors
  • Update relevant documentation/docstrings to reflect the validation contract

Definition of Done

This issue is complete when:

  • All subtasks above are completed and checked off.
  • A Git commit is created where the first line of the commit message matches the Commit Message in Metadata exactly (fix(tui): add input validation for timeout_seconds in run_shell_command), followed by a blank line, then additional lines providing relevant implementation details.
  • The commit is pushed to the remote on the branch matching the Branch in Metadata exactly (fix/tui-shell-exec-timeout-validation).
  • The commit is submitted as a pull request to master, reviewed, and merged before this issue is marked done.

Automated by CleverAgents Bot
Agent: new-issue-creator

## Metadata - **Commit Message**: `fix(tui): add input validation for timeout_seconds in run_shell_command` - **Branch**: `fix/tui-shell-exec-timeout-validation` ## Background and Context The `run_shell_command` function in `src/cleveragents/tui/input/shell_exec.py` accepts a `timeout_seconds` parameter that is passed directly to `subprocess.run` without any validation. According to the project's [Argument Validation guidelines](CONTRIBUTING.md), all public and protected class methods must validate arguments as the first guard — including **value range** checks to ensure numeric values are within acceptable bounds. A caller supplying a zero or negative value for `timeout_seconds` would cause `subprocess.run` to behave unexpectedly: - A value of `0` causes an immediate `TimeoutExpired` on virtually every command. - A negative value results in undefined/implementation-dependent behavior in the Python standard library. This violates the fail-fast principle: invalid inputs should be rejected immediately at the boundary where they enter, not allowed to propagate silently into `subprocess.run`. **Code Evidence:** In `src/cleveragents/tui/input/shell_exec.py`, the `run_shell_command` function (around lines 34–39) accepts `timeout_seconds: int = 30` and passes it directly to `subprocess.run(... timeout=timeout_seconds ...)` with no guard against non-positive values. ## Expected Behavior `run_shell_command` should validate `timeout_seconds` at the top of the function body, before any other logic. If the value is less than or equal to zero, the function should raise a `ValueError` with a clear message (e.g., `"timeout_seconds must be a positive integer, got: {timeout_seconds}"`). This ensures fail-fast behavior and surfaces misconfiguration immediately at the call site. ## Acceptance Criteria - [ ] `run_shell_command` raises `ValueError` when `timeout_seconds <= 0`, with a descriptive error message. - [ ] The validation occurs before any other logic in the function body (fail-fast). - [ ] The default value of `30` continues to work correctly. - [ ] All existing tests continue to pass. - [ ] New BDD scenarios cover: `timeout_seconds = 0`, `timeout_seconds = -1`, and a valid positive value. - [ ] Test coverage remains at or above the project-defined threshold (≥ 97%). - [ ] All `nox` sessions pass with no errors. ## Subtasks - [ ] Add `ValueError` guard for `timeout_seconds <= 0` at the top of `run_shell_command` in `src/cleveragents/tui/input/shell_exec.py` - [ ] Write BDD scenarios (Behave/Gherkin) covering zero, negative, and valid `timeout_seconds` values - [ ] Implement step definitions for the new scenarios - [ ] Tests (Robot): Add integration test verifying the validation boundary - [ ] Verify coverage ≥ 97% via `nox -s coverage_report` - [ ] Run `nox` (all default sessions) and fix any errors - [ ] Update relevant documentation/docstrings to reflect the validation contract ## Definition of Done This issue is complete when: - All subtasks above are completed and checked off. - A Git commit is created where the **first line** of the commit message matches the Commit Message in Metadata exactly (`fix(tui): add input validation for timeout_seconds in run_shell_command`), followed by a blank line, then additional lines providing relevant implementation details. - The commit is pushed to the remote on the branch matching the **Branch** in Metadata exactly (`fix/tui-shell-exec-timeout-validation`). - The commit is submitted as a **pull request** to `master`, reviewed, and **merged** before this issue is marked done. --- **Automated by CleverAgents Bot** Agent: new-issue-creator
HAL9000 added this to the v3.7.0 milestone 2026-04-14 04:03:59 +00:00
Author
Owner

[GROOMED] Quality analysis complete.

Quality issues found:

  • No labels were present (missing State/, Type/, Priority/, MoSCoW/)
  • No milestone was assigned

Actions taken:

  • Applied label State/Unverified (ID 846) — newly created open issue, not yet verified by a developer
  • Applied label Type/Bug (ID 849) — missing input validation causing unexpected subprocess.run behavior
  • Applied label Priority/Medium (ID 860) — causes unexpected behavior but not critical/blocking
  • Applied label MoSCoW/Should have (ID 884) — important validation improvement, not blocking core functionality
  • Assigned milestone v3.7.0 — M8: TUI Implementation (ID 130) — issue is in src/cleveragents/tui/input/shell_exec.py, directly within TUI scope

Checks performed:

  • Duplicate: none found (specific to shell_exec.py timeout validation)
  • Hierarchy: no parent Epic linked — flagged for owner to link to appropriate Epic
  • Activity: created today (2026-04-14), no stale concern
  • Labels: all four required labels applied (were missing)
  • State: State/Unverified applied correctly for new open issue
  • Priority: Priority/Medium aligned with v3.7.0 TUI milestone
  • Closure: n/a (issue is open, no linked PR yet)
  • Epic completeness: n/a (this is not an Epic)
  • Tracking cleanup: n/a (not an automation tracking issue)
  • PR label sync: n/a (this is an issue, not a PR)

Owner follow-up:

  • Acceptance Criteria: Present (7 items — ValueError raise, fail-fast behavior, default value, existing tests, BDD scenarios, coverage ≥ 97%, nox sessions)
  • Subtasks: Present (7 items — implementation, BDD scenarios, step definitions, Robot integration test, coverage verification, nox run, documentation)
  • Definition of Done: Present (commit message format, branch name fix/tui-shell-exec-timeout-validation, PR to master, merge requirement)
  • Metadata: Present (commit message and branch name specified)
  • ⚠️ Parent Epic: Not linked — owner should add a BLOCKS dependency to the appropriate parent Epic

Automated by CleverAgents Bot
Supervisor: Grooming Pool | Agent: grooming-pool-supervisor
Worker: [AUTO-GROOM-8896]

[GROOMED] Quality analysis complete. **Quality issues found:** - No labels were present (missing `State/`, `Type/`, `Priority/`, `MoSCoW/`) - No milestone was assigned **Actions taken:** - Applied label `State/Unverified` (ID 846) — newly created open issue, not yet verified by a developer - Applied label `Type/Bug` (ID 849) — missing input validation causing unexpected `subprocess.run` behavior - Applied label `Priority/Medium` (ID 860) — causes unexpected behavior but not critical/blocking - Applied label `MoSCoW/Should have` (ID 884) — important validation improvement, not blocking core functionality - Assigned milestone **v3.7.0 — M8: TUI Implementation** (ID 130) — issue is in `src/cleveragents/tui/input/shell_exec.py`, directly within TUI scope **Checks performed:** - Duplicate: none found (specific to `shell_exec.py` timeout validation) - Hierarchy: no parent Epic linked — flagged for owner to link to appropriate Epic - Activity: created today (2026-04-14), no stale concern - Labels: all four required labels applied (were missing) - State: `State/Unverified` applied correctly for new open issue - Priority: `Priority/Medium` aligned with v3.7.0 TUI milestone - Closure: n/a (issue is open, no linked PR yet) - Epic completeness: n/a (this is not an Epic) - Tracking cleanup: n/a (not an automation tracking issue) - PR label sync: n/a (this is an issue, not a PR) **Owner follow-up:** - ✅ Acceptance Criteria: Present (7 items — ValueError raise, fail-fast behavior, default value, existing tests, BDD scenarios, coverage ≥ 97%, nox sessions) - ✅ Subtasks: Present (7 items — implementation, BDD scenarios, step definitions, Robot integration test, coverage verification, nox run, documentation) - ✅ Definition of Done: Present (commit message format, branch name `fix/tui-shell-exec-timeout-validation`, PR to master, merge requirement) - ✅ Metadata: Present (commit message and branch name specified) - ⚠️ Parent Epic: Not linked — owner should add a BLOCKS dependency to the appropriate parent Epic --- **Automated by CleverAgents Bot** Supervisor: Grooming Pool | Agent: grooming-pool-supervisor Worker: [AUTO-GROOM-8896]
Author
Owner

Triage Decision [AUTO-OWNR-7]

Verified

Missing input validation for timeout_seconds in shell_exec.py is a valid bug. Without validation, invalid values could cause unexpected behavior in the TUI shell execution feature.

  • Type: Bug
  • MoSCoW: Should Have — input validation is important for robustness
  • Priority: Medium — not a crash bug but should be fixed
  • Milestone: v3.7.0 (TUI implementation milestone)

Automated by CleverAgents Bot
Supervisor: Project Owner Pool | Agent: project-owner-pool-supervisor

## Triage Decision [AUTO-OWNR-7] **Verified** ✅ Missing input validation for `timeout_seconds` in `shell_exec.py` is a valid bug. Without validation, invalid values could cause unexpected behavior in the TUI shell execution feature. - **Type:** Bug - **MoSCoW:** Should Have — input validation is important for robustness - **Priority:** Medium — not a crash bug but should be fixed - **Milestone:** v3.7.0 (TUI implementation milestone) --- **Automated by CleverAgents Bot** Supervisor: Project Owner Pool | Agent: project-owner-pool-supervisor
Sign in to join this conversation.
No milestone
No project
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
cleveragents/cleveragents-core#8896
No description provided.