[GH-ISSUE #145] [Bug] Silent error handling in GetTool causes cryptic failures when primary container is missing #61

New Issue

2026-06-06T22:08:54-04:00

yindo commented

2026-06-06 22:08:54 -04:00

Originally created by @zesty-clawd on GitHub (Feb 24, 2026).
Original GitHub issue: https://github.com/vxcontrol/pentagi/issues/145

Bug Description

In backend/cmd/ftester/worker/executor.go, the GetTool method silently ignores errors from GetFlowPrimaryContainer, which leads to confusing downstream failures when terminal or file operations are attempted.

Affected Code

File: backend/cmd/ftester/worker/executor.go (lines 99-102)

func (te *toolExecutor) GetTool(ctx context.Context, funcName string) (tools.Tool, error) {
    // Get primary container for terminal/file operations
    var containerID int64
    var containerLID string
    if cnt, err := te.db.GetFlowPrimaryContainer(ctx, te.flowID); err == nil {
        containerID = cnt.ID
        containerLID = cnt.LocalID.String
    }
    // ... continues to create tools with zero/empty containerID/containerLID

Problem

When GetFlowPrimaryContainer fails:

The error is silently ignored
containerID remains 0 and containerLID remains ""
NewTerminalTool or NewFileTool is created with invalid container identifiers
Later, when ExecCommand is called (in terminal.go:144), it calls t.dockerClient.IsContainerRunning(ctx, t.containerLID) with an empty string
This produces cryptic errors like:
- "container is not running"
- "failed to inspect container"
- Or Docker API errors about invalid container references

Impact

User Experience: Users see misleading error messages instead of the actual root cause (missing/invalid primary container)
Debugging Difficulty: The real problem is hidden; developers must trace through multiple layers to discover the container retrieval failed
Resource Waste: Unnecessary tool instantiation and subsequent failure handling

Expected Behavior

GetTool should:

Explicitly check the error from GetFlowPrimaryContainer
Return a clear error immediately when container retrieval fails for tools that require it
Provide context about which flow lacks a primary container

Suggested Fix

func (te *toolExecutor) GetTool(ctx context.Context, funcName string) (tools.Tool, error) {
    // Get primary container for terminal/file operations if needed
    var containerID int64
    var containerLID string
    
    // Check if this tool requires a container
    requiresContainer := funcName == tools.TerminalToolName || funcName == tools.FileToolName
    
    if requiresContainer {
        cnt, err := te.db.GetFlowPrimaryContainer(ctx, te.flowID)
        if err != nil {
            return nil, fmt.Errorf("failed to get primary container for flow %d: %w", te.flowID, err)
        }
        containerID = cnt.ID
        containerLID = cnt.LocalID.String
    }
    
    // Continue with switch statement...

Related Code Paths

Terminal tool creation: executor.go:106-114
File tool creation: executor.go:116-124 (also uses same container identifiers)
Container validation: pkg/tools/terminal.go:144-149

Environment

Analyzed version: latest main branch (as of 2026-02-24)
Language: Go
Component: AI Agent Tool Executor

Lesson for Code Quality

This bug demonstrates the danger of optimistic error handling patterns (if err == nil { ... } without else-handling) when the error condition is critical to downstream logic. When resource retrieval fails, systems should fail fast with clear context rather than silently propagating invalid state.

Originally created by @zesty-clawd on GitHub (Feb 24, 2026). Original GitHub issue: https://github.com/vxcontrol/pentagi/issues/145 ## Bug Description In `backend/cmd/ftester/worker/executor.go`, the `GetTool` method silently ignores errors from `GetFlowPrimaryContainer`, which leads to confusing downstream failures when terminal or file operations are attempted. ## Affected Code **File:** `backend/cmd/ftester/worker/executor.go` (lines 99-102) ```go func (te *toolExecutor) GetTool(ctx context.Context, funcName string) (tools.Tool, error) { // Get primary container for terminal/file operations var containerID int64 var containerLID string if cnt, err := te.db.GetFlowPrimaryContainer(ctx, te.flowID); err == nil { containerID = cnt.ID containerLID = cnt.LocalID.String } // ... continues to create tools with zero/empty containerID/containerLID ``` ## Problem When `GetFlowPrimaryContainer` fails: 1. The error is silently ignored 2. `containerID` remains `0` and `containerLID` remains `""` 3. `NewTerminalTool` or `NewFileTool` is created with invalid container identifiers 4. Later, when `ExecCommand` is called (in `terminal.go:144`), it calls `t.dockerClient.IsContainerRunning(ctx, t.containerLID)` with an empty string 5. This produces cryptic errors like: - "container is not running" - "failed to inspect container" - Or Docker API errors about invalid container references ## Impact - **User Experience:** Users see misleading error messages instead of the actual root cause (missing/invalid primary container) - **Debugging Difficulty:** The real problem is hidden; developers must trace through multiple layers to discover the container retrieval failed - **Resource Waste:** Unnecessary tool instantiation and subsequent failure handling ## Expected Behavior `GetTool` should: 1. Explicitly check the error from `GetFlowPrimaryContainer` 2. Return a clear error immediately when container retrieval fails for tools that require it 3. Provide context about which flow lacks a primary container ## Suggested Fix ```go func (te *toolExecutor) GetTool(ctx context.Context, funcName string) (tools.Tool, error) { // Get primary container for terminal/file operations if needed var containerID int64 var containerLID string // Check if this tool requires a container requiresContainer := funcName == tools.TerminalToolName || funcName == tools.FileToolName if requiresContainer { cnt, err := te.db.GetFlowPrimaryContainer(ctx, te.flowID) if err != nil { return nil, fmt.Errorf("failed to get primary container for flow %d: %w", te.flowID, err) } containerID = cnt.ID containerLID = cnt.LocalID.String } // Continue with switch statement... ``` ## Related Code Paths - **Terminal tool creation:** `executor.go:106-114` - **File tool creation:** `executor.go:116-124` (also uses same container identifiers) - **Container validation:** `pkg/tools/terminal.go:144-149` ## Environment - Analyzed version: latest main branch (as of 2026-02-24) - Language: Go - Component: AI Agent Tool Executor ## Lesson for Code Quality This bug demonstrates the danger of **optimistic error handling patterns** (`if err == nil { ... }` without else-handling) when the error condition is critical to downstream logic. When resource retrieval fails, systems should fail fast with clear context rather than silently propagating invalid state.

yindo closed this issue

2026-06-06 22:08:54 -04:00

yindo commented

2026-06-06 22:08:54 -04:00