Enhance test quality in mcp_utilities_test.go with security cases and fail-fast patterns

[Copilot]

Improves test quality for shell quoting and Docker command building functions by adding security-focused edge cases and applying fail-fast assertion patterns.

Changes

• Fail-fast pattern: Changed first assertion in TestBuildDockerCommandWithExpandableVars_PreservesVariableExpansion from assert.Contains to require.Contains to stop execution immediately if variable preservation fails

• Security edge cases: Added test cases for command injection attempts:

  • Semicolon injection: "file; rm -rf /"
  • Pipe injection: "file | cat /etc/passwd"
  • Multiple quote escaping: "it's a test's file"
  • GITHUB_WORKSPACE injection: "${GITHUB_WORKSPACE}; rm -rf /"

• Unbraced variable behavior: Added TestBuildDockerCommandWithExpandableVars_UnbracedVariable to document that $GITHUB_WORKSPACE (without braces) is quoted as a literal, not preserved for expansion

Example

// Before: continues checking even if variable not preserved
assert.Contains(t, result, "${GITHUB_WORKSPACE}", "...")
assert.Contains(t, result, "\"${GITHUB_WORKSPACE}\"", "...")

// After: fails immediately if variable not preserved
require.Contains(t, result, "${GITHUB_WORKSPACE}", "...")
assert.Contains(t, result, "\"${GITHUB_WORKSPACE}\"", "...")

All security test cases verify that malicious input is properly quoted and neutralized.

Original prompt

---

This section details on the original issue you should resolve

<issue_title>[testify-expert] Improve Test Quality: pkg/workflow/mcp_utilities_test.go</issue_title>
<issue_description>### Overview

The test file pkg/workflow/mcp_utilities_test.go has been selected for quality review by the Testify Uber Super Expert. This file demonstrates excellent testify practices with comprehensive table-driven tests and proper assertion usage. This review identifies the file's strengths and suggests minor enhancements to achieve perfection.

Current State

• Test File: pkg/workflow/mcp_utilities_test.go
• Source File: pkg/workflow/mcp_utilities.go
• Test Functions: 3 test functions
• Lines of Code: 151 lines (test), 44 lines (source)
• Last Modified: 2026-01-26
• Test Coverage: ✅ 100% - All 2 functions in source file are tested

Test Quality Analysis

Strengths ✅

This test file is a model example of testify best practices and should be referenced as a template for other tests:

1. Perfect table-driven test structure - Uses comprehensive test tables with descriptive names covering edge cases
2. Proper testify assertion usage - Uses assert.Equal() and assert.Contains() with helpful messages
3. Excellent test organization - Uses t.Run() for subtests with clear, descriptive names
4. Complete coverage - Tests all functions (100% coverage) including edge cases like empty strings, special characters, and complex scenarios
5. Good test separation - Separate test for specific behavior (TestBuildDockerCommandWithExpandableVars_PreservesVariableExpansion)

Areas for Minor Enhancement 🎯

1. Add require.* for Critical Checks in Preserve Test

Current State:
The TestBuildDockerCommandWithExpandableVars_PreservesVariableExpansion function uses only assert.* assertions. While this works, using require.* for the first assertion would make the test fail-fast if the fundamental requirement isn't met.

Current Code:

func TestBuildDockerCommandWithExpandableVars_PreservesVariableExpansion(t *testing.T) {
    input := "docker run -v ${GITHUB_WORKSPACE}:/workspace"
    result := buildDockerCommandWithExpandableVars(input)

    // Both assertions use assert.*
    assert.Contains(t, result, "${GITHUB_WORKSPACE}", "Result should preserve GITHUB_WORKSPACE variable for expansion")
    assert.Contains(t, result, "\"${GITHUB_WORKSPACE}\"", "GITHUB_WORKSPACE should be in double quotes for safe expansion")
}

Recommended Change:

func TestBuildDockerCommandWithExpandableVars_PreservesVariableExpansion(t *testing.T) {
    input := "docker run -v ${GITHUB_WORKSPACE}:/workspace"
    result := buildDockerCommandWithExpandableVars(input)

    // Use require.* for the first critical check (fail-fast)
    require.Contains(t, result, "${GITHUB_WORKSPACE}", "Result should preserve GITHUB_WORKSPACE variable for expansion")
    
    // Use assert.* for the second check (still runs if we get here)
    assert.Contains(t, result, "\"${GITHUB_WORKSPACE}\"", "GITHUB_WORKSPACE should be in double quotes for safe expansion")
}

Why this matters: Following the pattern from specs/testing.md, use require.* for critical setup/preconditions that must pass before continuing. This makes test failures clearer - if the variable isn't preserved at all (first check), we don't need to check the quoting format (second check).

2. Consider Adding Negative Test Cases

Current State:
The tests comprehensively cover positive cases (valid inputs) and edge cases. Consider adding explicit tests for security-related edge cases.

Potential Additional Test Cases:

// Add to TestShellQuote table
{
    name:     "command injection attempt with semicolon",
    input:    "file; rm -rf /",
    expected: "'file; rm -rf /'",
},
{
    name:     "command injection attempt with pipe",
    input:    "file | cat /etc/passwd",
    expected: "'file | cat /etc/passwd'",
},
{
    name:     "multiple single quotes",
    input:    "it's a test's file",
    expected: "'it'\\''s a test'\\''s file'",
},

// Add to TestBuildDockerCommandWithExpandableVars table
{
    name:     "injection attempt in GITHUB_WORKSPACE context",
    input:    "${GITHUB_WORKSPACE}; rm -rf /",
    expected: "''\"${GITHUB_WORKSPACE}\"'; rm -rf /'",
},
{
    name:     "multiple variables mixed with GITHUB_WORKSPACE",
    input:    "${GITHUB_WORKSPACE}/src ${OTHER_VAR}/dst",
    expected: "''\"${GITHUB_WORKSPACE}\"'/src ${OTHER_VAR}/dst'",
},

Why this matters: These functions are used for shell quoting and Docker command construction - security-critical operations. Explicitly testing injection attempts documents the security behavior and prevents regressions.

3. Add Test for $GITHUB_WORKSPACE (Without Braces)

Current State:
The source file comment mentions both ${GITHUB_WORKSPACE} (with braces) and $GITHUB_WORKSPACE (without braces):

// buildD...

</details>



<!-- START COPILOT CODING AGENT SUFFIX -->

- Fixes githubnext/gh-aw#11851

<!-- START COPILOT CODING AGENT TIPS -->
---

✨ Let Copilot coding agent [set things up for you](https://github.com/githubnext/gh-aw/issues/new?title=✨+Set+up+Copilot+instructions&body=Configure%20instructions%20for%20this%20repository%20as%20documented%20in%20%5BBest%20practices%20for%20Copilot%20coding%20agent%20in%20your%20repository%5D%28https://gh.io/copilot-coding-agent-tips%29%2E%0A%0A%3COnboard%20this%20repo%3E&assignees=copilot) — coding agent works faster and does higher quality work when set up for your repo.