github/Fabric
1
1
Fork 0
mirror of https://github.com/danielmiessler/Fabric.git synced 2026-01-09 22:38:10 -05:00
Code Issues Packages Projects Releases Wiki Activity

refactor: address PR review feedback

Browse Source 
- Extract InputSentinel constant to shared constants.go file
- Remove duplicate inputSentinel definitions from template.go and patterns.go
- Create withTestExtension helper function to reduce test code duplication
- Refactor 3 test functions to use the helper (reduces ~40 lines per test)
- Fix shell script to use $@ instead of $* for proper argument quoting

Addresses review comments from @ksylvan and @Copilot AI
This commit is contained in:
Nick Skriloff
2025-10-31 13:27:38 -04:00
parent eb1cfe8340
commit 4b82534708
4 changed files with 148 additions and 233 deletions
Download Patch File Download Diff File Expand all files Collapse all files

6
internal/plugins/db/fsdb/patterns.go
View File

@@ -11,8 +11,6 @@ import (
"github.com/danielmiessler/fabric/internal/util" "github.com/danielmiessler/fabric/internal/util"
) )
const inputSentinel = "__FABRIC_INPUT_SENTINEL_TOKEN__"
type PatternsEntity struct { type PatternsEntity struct {
*StorageEntity *StorageEntity
SystemPatternFile string SystemPatternFile string
@@ -96,7 +94,7 @@ func (o *PatternsEntity) applyVariables(
// Temporarily replace {{input}} with a sentinel token to protect it // Temporarily replace {{input}} with a sentinel token to protect it
// from recursive variable resolution // from recursive variable resolution
withSentinel := strings.ReplaceAll(pattern.Pattern, "{{input}}", inputSentinel) withSentinel := strings.ReplaceAll(pattern.Pattern, "{{input}}", template.InputSentinel)
// Process all other template variables in the pattern // Process all other template variables in the pattern
// Pass the actual input so extension calls can use {{input}} within their value parameter // Pass the actual input so extension calls can use {{input}} within their value parameter
@@ -107,7 +105,7 @@ func (o *PatternsEntity) applyVariables(
// Finally, replace our sentinel with the actual user input // Finally, replace our sentinel with the actual user input
// The input has already been processed for variables if InputHasVars was true // The input has already been processed for variables if InputHasVars was true
pattern.Pattern = strings.ReplaceAll(processed, inputSentinel, input) pattern.Pattern = strings.ReplaceAll(processed, template.InputSentinel, input)
return return
} }

5
internal/plugins/template/constants.go Normal file
View File

@@ -0,0 +1,5 @@
package template
// InputSentinel is used to temporarily replace {{input}} during template processing
// to prevent recursive variable resolution
const InputSentinel = "__FABRIC_INPUT_SENTINEL_TOKEN__"

10
internal/plugins/template/template.go
View File

@@ -10,10 +10,6 @@ import (
debuglog "github.com/danielmiessler/fabric/internal/log" debuglog "github.com/danielmiessler/fabric/internal/log"
) )
// inputSentinel is used to temporarily replace {{input}} during template processing
// to prevent recursive variable resolution
const inputSentinel = "__FABRIC_INPUT_SENTINEL_TOKEN__"
var ( var (
textPlugin = &TextPlugin{} textPlugin = &TextPlugin{}
datetimePlugin = &DateTimePlugin{} datetimePlugin = &DateTimePlugin{}
@@ -77,8 +73,8 @@ func ApplyTemplate(content string, variables map[string]string, input string) (s
// Extension call // Extension call
if strings.HasPrefix(raw, "ext:") { if strings.HasPrefix(raw, "ext:") {
if name, operation, value, ok := matchTriple(extensionPattern, full); ok { if name, operation, value, ok := matchTriple(extensionPattern, full); ok {
if strings.Contains(value, inputSentinel) { if strings.Contains(value, InputSentinel) {
value = strings.ReplaceAll(value, inputSentinel, input) value = strings.ReplaceAll(value, InputSentinel, input)
debugf("Replaced sentinel in extension value with input\n") debugf("Replaced sentinel in extension value with input\n")
} }
debugf("Extension call: name=%s operation=%s value=%s\n", name, operation, value) debugf("Extension call: name=%s operation=%s value=%s\n", name, operation, value)
@@ -132,7 +128,7 @@ func ApplyTemplate(content string, variables map[string]string, input string) (s
// Variables / input / sentinel // Variables / input / sentinel
switch raw { switch raw {
case "input", inputSentinel: case "input", InputSentinel:
content = strings.ReplaceAll(content, full, input) content = strings.ReplaceAll(content, full, input)
progress = true progress = true
default: default:

360
internal/plugins/template/template_sentinel_test.go
View File

@@ -1,16 +1,17 @@
package template package template
import ( import (
"fmt"
"os" "os"
"path/filepath" "path/filepath"
"strings" "strings"
"testing" "testing"
) )
// TestSentinelTokenReplacement tests the fix for the {{input}} sentinel token bug // withTestExtension creates a temporary test extension and runs the test function
// This test verifies that when {{input}} is used inside an extension call, func withTestExtension(t *testing.T, name string, scriptContent string, testFunc func(*ExtensionManager, string)) {
// the actual input is passed to the extension, not the sentinel token. t.Helper()
func TestSentinelTokenReplacement(t *testing.T) {
// Create a temporary directory for test extension // Create a temporary directory for test extension
tmpDir := t.TempDir() tmpDir := t.TempDir()
configDir := filepath.Join(tmpDir, ".config", "fabric") configDir := filepath.Join(tmpDir, ".config", "fabric")
@@ -27,23 +28,20 @@ func TestSentinelTokenReplacement(t *testing.T) {
t.Fatalf("Failed to create configs directory: %v", err) t.Fatalf("Failed to create configs directory: %v", err)
} }
// Create a test script that echoes what it receives // Create a test script
scriptPath := filepath.Join(binDir, "echo-test.sh") scriptPath := filepath.Join(binDir, name+".sh")
scriptContent := `#!/bin/bash
echo "RECEIVED: $*"
`
err = os.WriteFile(scriptPath, []byte(scriptContent), 0755) err = os.WriteFile(scriptPath, []byte(scriptContent), 0755)
if err != nil { if err != nil {
t.Fatalf("Failed to create test script: %v", err) t.Fatalf("Failed to create test script: %v", err)
} }
// Create extension config // Create extension config
configPath := filepath.Join(configsDir, "echo-test.yaml") configPath := filepath.Join(configsDir, name+".yaml")
configContent := `name: echo-test configContent := fmt.Sprintf(`name: %s
executable: ` + scriptPath + ` executable: %s
type: executable type: executable
timeout: "5s" timeout: "5s"
description: "Echo test extension" description: "Test extension"
version: "1.0.0" version: "1.0.0"
operations: operations:
@@ -53,80 +51,96 @@ operations:
config: config:
output: output:
method: stdout method: stdout
` `, name, scriptPath)
err = os.WriteFile(configPath, []byte(configContent), 0644) err = os.WriteFile(configPath, []byte(configContent), 0644)
if err != nil { if err != nil {
t.Fatalf("Failed to create extension config: %v", err) t.Fatalf("Failed to create extension config: %v", err)
} }
// Initialize extension manager with test config directory // Initialize extension manager with test config directory
oldManager := extensionManager mgr := NewExtensionManager(configDir)
defer func() { extensionManager = oldManager }()
extensionManager = NewExtensionManager(configDir)
// Register the test extension // Register the test extension
err = extensionManager.RegisterExtension(configPath) err = mgr.RegisterExtension(configPath)
if err != nil { if err != nil {
t.Fatalf("Failed to register extension: %v", err) t.Fatalf("Failed to register extension: %v", err)
} }
tests := []struct { // Run the test
name string testFunc(mgr, name)
template string }
input string
wantContain string
wantNotContain string
}{
{
name: "sentinel token with {{input}} in extension value",
template: "{{ext:echo-test:echo:__FABRIC_INPUT_SENTINEL_TOKEN__}}",
input: "test input data",
wantContain: "RECEIVED: test input data",
wantNotContain: "__FABRIC_INPUT_SENTINEL_TOKEN__",
},
{
name: "direct input variable replacement",
template: "{{ext:echo-test:echo:{{input}}}}",
input: "Hello World",
wantContain: "RECEIVED: Hello World",
wantNotContain: "{{input}}",
},
{
name: "sentinel with complex input",
template: "Result: {{ext:echo-test:echo:__FABRIC_INPUT_SENTINEL_TOKEN__}}",
input: "What is AI?",
wantContain: "RECEIVED: What is AI?",
wantNotContain: "__FABRIC_INPUT_SENTINEL_TOKEN__",
},
{
name: "multiple words in input",
template: "{{ext:echo-test:echo:{{input}}}}",
input: "Multiple word input string",
wantContain: "RECEIVED: Multiple word input string",
wantNotContain: "{{input}}",
},
}
for _, tt := range tests { // TestSentinelTokenReplacement tests the fix for the {{input}} sentinel token bug
t.Run(tt.name, func(t *testing.T) { // This test verifies that when {{input}} is used inside an extension call,
got, err := ApplyTemplate(tt.template, map[string]string{}, tt.input) // the actual input is passed to the extension, not the sentinel token.
if err != nil { func TestSentinelTokenReplacement(t *testing.T) {
t.Errorf("ApplyTemplate() error = %v", err) scriptContent := `#!/bin/bash
return echo "RECEIVED: $@"
} `
// Check that result contains expected string withTestExtension(t, "echo-test", scriptContent, func(mgr *ExtensionManager, name string) {
if !strings.Contains(got, tt.wantContain) { // Save and restore global extension manager
t.Errorf("ApplyTemplate() = %q, should contain %q", got, tt.wantContain) oldManager := extensionManager
} defer func() { extensionManager = oldManager }()
extensionManager = mgr
// Check that result does NOT contain unwanted string tests := []struct {
if strings.Contains(got, tt.wantNotContain) { name string
t.Errorf("ApplyTemplate() = %q, should NOT contain %q", got, tt.wantNotContain) template string
} input string
}) wantContain string
} wantNotContain string
}{
{
name: "sentinel token with {{input}} in extension value",
template: "{{ext:echo-test:echo:__FABRIC_INPUT_SENTINEL_TOKEN__}}",
input: "test input data",
wantContain: "RECEIVED: test input data",
wantNotContain: "__FABRIC_INPUT_SENTINEL_TOKEN__",
},
{
name: "direct input variable replacement",
template: "{{ext:echo-test:echo:{{input}}}}",
input: "Hello World",
wantContain: "RECEIVED: Hello World",
wantNotContain: "{{input}}",
},
{
name: "sentinel with complex input",
template: "Result: {{ext:echo-test:echo:__FABRIC_INPUT_SENTINEL_TOKEN__}}",
input: "What is AI?",
wantContain: "RECEIVED: What is AI?",
wantNotContain: "__FABRIC_INPUT_SENTINEL_TOKEN__",
},
{
name: "multiple words in input",
template: "{{ext:echo-test:echo:{{input}}}}",
input: "Multiple word input string",
wantContain: "RECEIVED: Multiple word input string",
wantNotContain: "{{input}}",
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
got, err := ApplyTemplate(tt.template, map[string]string{}, tt.input)
if err != nil {
t.Errorf("ApplyTemplate() error = %v", err)
return
}
// Check that result contains expected string
if !strings.Contains(got, tt.wantContain) {
t.Errorf("ApplyTemplate() = %q, should contain %q", got, tt.wantContain)
}
// Check that result does NOT contain unwanted string
if strings.Contains(got, tt.wantNotContain) {
t.Errorf("ApplyTemplate() = %q, should NOT contain %q", got, tt.wantNotContain)
}
})
}
})
} }
// TestSentinelInVariableProcessing tests that the sentinel token is handled // TestSentinelInVariableProcessing tests that the sentinel token is handled
@@ -180,180 +194,82 @@ func TestSentinelInVariableProcessing(t *testing.T) {
// TestExtensionValueWithSentinel specifically tests the extension value // TestExtensionValueWithSentinel specifically tests the extension value
// sentinel replacement logic // sentinel replacement logic
func TestExtensionValueWithSentinel(t *testing.T) { func TestExtensionValueWithSentinel(t *testing.T) {
// Create a temporary directory for test extension
tmpDir := t.TempDir()
configDir := filepath.Join(tmpDir, ".config", "fabric")
extensionsDir := filepath.Join(configDir, "extensions")
binDir := filepath.Join(extensionsDir, "bin")
configsDir := filepath.Join(extensionsDir, "configs")
err := os.MkdirAll(binDir, 0755)
if err != nil {
t.Fatalf("Failed to create bin directory: %v", err)
}
err = os.MkdirAll(configsDir, 0755)
if err != nil {
t.Fatalf("Failed to create configs directory: %v", err)
}
// Create a test script that outputs the exact arguments it receives
scriptPath := filepath.Join(binDir, "arg-test.sh")
scriptContent := `#!/bin/bash scriptContent := `#!/bin/bash
# Output each argument on a separate line # Output each argument on a separate line
for arg in "$@"; do for arg in "$@"; do
echo "ARG: $arg" echo "ARG: $arg"
done done
` `
err = os.WriteFile(scriptPath, []byte(scriptContent), 0755)
if err != nil {
t.Fatalf("Failed to create test script: %v", err)
}
// Create extension config withTestExtension(t, "arg-test", scriptContent, func(mgr *ExtensionManager, name string) {
configPath := filepath.Join(configsDir, "arg-test.yaml") // Save and restore global extension manager
configContent := `name: arg-test oldManager := extensionManager
executable: ` + scriptPath + ` defer func() { extensionManager = oldManager }()
type: executable extensionManager = mgr
timeout: "5s"
description: "Argument test extension"
version: "1.0.0"
operations: // Test that sentinel token in extension value gets replaced
test: template := "{{ext:arg-test:echo:prefix-__FABRIC_INPUT_SENTINEL_TOKEN__-suffix}}"
cmd_template: "{{executable}} {{value}}" input := "MYINPUT"
config: got, err := ApplyTemplate(template, map[string]string{}, input)
output: if err != nil {
method: stdout t.Fatalf("ApplyTemplate() error = %v", err)
` }
err = os.WriteFile(configPath, []byte(configContent), 0644)
if err != nil {
t.Fatalf("Failed to create extension config: %v", err)
}
// Initialize extension manager with test config directory // The sentinel should be replaced with actual input
oldManager := extensionManager expectedContain := "ARG: prefix-MYINPUT-suffix"
defer func() { extensionManager = oldManager }() if !strings.Contains(got, expectedContain) {
t.Errorf("ApplyTemplate() = %q, should contain %q", got, expectedContain)
}
extensionManager = NewExtensionManager(configDir) // The sentinel token should NOT appear in output
if strings.Contains(got, "__FABRIC_INPUT_SENTINEL_TOKEN__") {
// Register the test extension t.Errorf("ApplyTemplate() = %q, should NOT contain sentinel token", got)
err = extensionManager.RegisterExtension(configPath) }
if err != nil { })
t.Fatalf("Failed to register extension: %v", err)
}
// Test that sentinel token in extension value gets replaced
template := "{{ext:arg-test:test:prefix-__FABRIC_INPUT_SENTINEL_TOKEN__-suffix}}"
input := "MYINPUT"
got, err := ApplyTemplate(template, map[string]string{}, input)
if err != nil {
t.Fatalf("ApplyTemplate() error = %v", err)
}
// The sentinel should be replaced with actual input
expectedContain := "ARG: prefix-MYINPUT-suffix"
if !strings.Contains(got, expectedContain) {
t.Errorf("ApplyTemplate() = %q, should contain %q", got, expectedContain)
}
// The sentinel token should NOT appear in output
if strings.Contains(got, "__FABRIC_INPUT_SENTINEL_TOKEN__") {
t.Errorf("ApplyTemplate() = %q, should NOT contain sentinel token", got)
}
} }
// TestNestedInputInExtension tests the original bug case: // TestNestedInputInExtension tests the original bug case:
// {{ext:name:op:{{input}}}} should pass the actual input, not the sentinel // {{ext:name:op:{{input}}}} should pass the actual input, not the sentinel
func TestNestedInputInExtension(t *testing.T) { func TestNestedInputInExtension(t *testing.T) {
// Create a temporary directory for test extension
tmpDir := t.TempDir()
configDir := filepath.Join(tmpDir, ".config", "fabric")
extensionsDir := filepath.Join(configDir, "extensions")
binDir := filepath.Join(extensionsDir, "bin")
configsDir := filepath.Join(extensionsDir, "configs")
err := os.MkdirAll(binDir, 0755)
if err != nil {
t.Fatalf("Failed to create bin directory: %v", err)
}
err = os.MkdirAll(configsDir, 0755)
if err != nil {
t.Fatalf("Failed to create configs directory: %v", err)
}
// Create a test script
scriptPath := filepath.Join(binDir, "nested-test.sh")
scriptContent := `#!/bin/bash scriptContent := `#!/bin/bash
echo "NESTED_TEST: $*" echo "NESTED_TEST: $*"
` `
err = os.WriteFile(scriptPath, []byte(scriptContent), 0755)
if err != nil {
t.Fatalf("Failed to create test script: %v", err)
}
// Create extension config withTestExtension(t, "nested-test", scriptContent, func(mgr *ExtensionManager, name string) {
configPath := filepath.Join(configsDir, "nested-test.yaml") // Save and restore global extension manager
configContent := `name: nested-test oldManager := extensionManager
executable: ` + scriptPath + ` defer func() { extensionManager = oldManager }()
type: executable extensionManager = mgr
timeout: "5s"
description: "Nested input test extension"
version: "1.0.0"
operations: // This is the bug case: {{input}} nested inside extension call
process: // The template processing should:
cmd_template: "{{executable}} {{value}}" // 1. Replace {{input}} with sentinel during variable protection
// 2. Process the extension, replacing sentinel with actual input
// 3. Execute extension with actual input, not sentinel
config: template := "{{ext:nested-test:echo:{{input}}}}"
output: input := "What is Artificial Intelligence"
method: stdout
`
err = os.WriteFile(configPath, []byte(configContent), 0644)
if err != nil {
t.Fatalf("Failed to create extension config: %v", err)
}
// Initialize extension manager with test config directory got, err := ApplyTemplate(template, map[string]string{}, input)
oldManager := extensionManager if err != nil {
defer func() { extensionManager = oldManager }() t.Fatalf("ApplyTemplate() error = %v", err)
}
extensionManager = NewExtensionManager(configDir) // Verify the actual input was passed, not the sentinel
expectedContain := "NESTED_TEST: What is Artificial Intelligence"
if !strings.Contains(got, expectedContain) {
t.Errorf("ApplyTemplate() = %q, should contain %q", got, expectedContain)
}
// Register the test extension // Verify sentinel token does NOT appear
err = extensionManager.RegisterExtension(configPath) if strings.Contains(got, "__FABRIC_INPUT_SENTINEL_TOKEN__") {
if err != nil { t.Errorf("ApplyTemplate() output contains sentinel token (BUG NOT FIXED): %q", got)
t.Fatalf("Failed to register extension: %v", err) }
}
// This is the bug case: {{input}} nested inside extension call // Verify {{input}} template tag does NOT appear
// The template processing should: if strings.Contains(got, "{{input}}") {
// 1. Replace {{input}} with sentinel during variable protection t.Errorf("ApplyTemplate() output contains unresolved {{input}}: %q", got)
// 2. Process the extension, replacing sentinel with actual input }
// 3. Execute extension with actual input, not sentinel })
template := "{{ext:nested-test:process:{{input}}}}"
input := "What is Artificial Intelligence"
got, err := ApplyTemplate(template, map[string]string{}, input)
if err != nil {
t.Fatalf("ApplyTemplate() error = %v", err)
}
// Verify the actual input was passed, not the sentinel
expectedContain := "NESTED_TEST: What is Artificial Intelligence"
if !strings.Contains(got, expectedContain) {
t.Errorf("ApplyTemplate() = %q, should contain %q", got, expectedContain)
}
// Verify sentinel token does NOT appear
if strings.Contains(got, "__FABRIC_INPUT_SENTINEL_TOKEN__") {
t.Errorf("ApplyTemplate() output contains sentinel token (BUG NOT FIXED): %q", got)
}
// Verify {{input}} template tag does NOT appear
if strings.Contains(got, "{{input}}") {
t.Errorf("ApplyTemplate() output contains unresolved {{input}}: %q", got)
}
} }