Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 0 additions & 9 deletions pkg/cli/mcp_helpers.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ package cli
import (
"fmt"
"os"
"os/exec"
"path/filepath"
"strings"

Expand Down Expand Up @@ -85,11 +84,3 @@ func withNonInteractiveCIEnv(env []string) []string {

return append(env, "CI=1")
}

func setNonInteractiveCIEnv(cmd *exec.Cmd) {
if cmd == nil {
return
}

cmd.Env = withNonInteractiveCIEnv(cmd.Env)
}
24 changes: 6 additions & 18 deletions pkg/cli/mcp_validation_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ package cli

import (
"os"
"os/exec"
"path/filepath"
"testing"

Expand Down Expand Up @@ -61,7 +60,7 @@ func TestGetBinaryPath(t *testing.T) {
})
}

func TestSetNonInteractiveCIEnv(t *testing.T) {
func TestWithNonInteractiveCIEnv(t *testing.T) {
t.Run("returns copied env with CI forced on", func(t *testing.T) {
input := []string{"CI=false", "HOME=/tmp/test-home"}

Expand All @@ -74,23 +73,12 @@ func TestSetNonInteractiveCIEnv(t *testing.T) {
})

t.Run("adds CI when missing", func(t *testing.T) {
cmd := exec.Command("echo")
cmd.Env = []string{"HOME=/tmp/test-home"}
input := []string{"HOME=/tmp/test-home"}

setNonInteractiveCIEnv(cmd)

assert.Contains(t, cmd.Env, "CI=1")
assert.Contains(t, cmd.Env, "HOME=/tmp/test-home")
})

t.Run("overrides existing CI value", func(t *testing.T) {
cmd := exec.Command("echo")
cmd.Env = []string{"CI=false", "HOME=/tmp/test-home"}

setNonInteractiveCIEnv(cmd)
output := withNonInteractiveCIEnv(input)

assert.Contains(t, cmd.Env, "CI=1")
assert.NotContains(t, cmd.Env, "CI=false")
assert.Contains(t, cmd.Env, "HOME=/tmp/test-home")
assert.Equal(t, []string{"HOME=/tmp/test-home"}, input, "must not mutate input")
assert.Contains(t, output, "CI=1")
assert.Contains(t, output, "HOME=/tmp/test-home")
})
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Test coverage regression: the withNonInteractiveCIEnv append-fallback path (return append(env, "CI=1")) is no longer exercised by any test case.

💡 Why this matters / suggested fix

Before this PR, setNonInteractiveCIEnv wrapped withNonInteractiveCIEnv, and its now-deleted "adds CI when missing" sub-test indirectly covered the branch in withNonInteractiveCIEnv that appends CI=1 when no CI= entry is present in the slice.

The remaining sub-test only covers the replacement branch (input already contains CI=false). After this removal, the append(env, "CI=1") line in withNonInteractiveCIEnv has no direct test.

Consider adding a sub-test to maintain coverage:

t.Run("adds CI when missing", func(t *testing.T) {
    input := []string{"HOME=/tmp/test-home"}

    output := withNonInteractiveCIEnv(input)

    assert.Equal(t, []string{"HOME=/tmp/test-home"}, input, "must not mutate input")
    assert.Contains(t, output, "CI=1")
    assert.Contains(t, output, "HOME=/tmp/test-home")
})

68 changes: 0 additions & 68 deletions pkg/cli/outcome_domain_breakdown.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ package cli

import (
"cmp"
"fmt"
"slices"
"strings"

Expand Down Expand Up @@ -111,70 +110,3 @@ func countTotalAttempted(breakdowns []DomainBreakdown) int {
}
return total
}

// DomainInsight provides a human-readable assessment of a domain's performance.
type DomainInsight struct {
Label string
Status string // "excellent", "good", "fair", "poor", "new"
Efficiency float64
Message string
Suggestion string
}

// AnalyzeDomainPerformance provides strategic insights about domain efficiency.
func AnalyzeDomainPerformance(breakdown DomainBreakdown) DomainInsight {
insight := DomainInsight{
Label: breakdown.Label,
Efficiency: breakdown.ObjectiveEfficiency,
}

if breakdown.Attempted == 0 {
insight.Status = "new"
insight.Message = "No outcomes yet"
return insight
}

switch {
case breakdown.ObjectiveEfficiency >= 0.90:
insight.Status = "excellent"
insight.Message = fmt.Sprintf("✅ %s: %.0f%% efficiency | %d/%d outcomes accepted | %d value delivered",
breakdown.Label,
breakdown.ObjectiveEfficiency*100,
breakdown.Accepted,
breakdown.Attempted,
breakdown.AcceptedObjectiveValue)
insight.Suggestion = "Keep current strategy working well"

case breakdown.ObjectiveEfficiency >= 0.75:
insight.Status = "good"
insight.Message = fmt.Sprintf("✅ %s: %.0f%% efficiency | %d/%d outcomes accepted | %d value delivered",
breakdown.Label,
breakdown.ObjectiveEfficiency*100,
breakdown.Accepted,
breakdown.Attempted,
breakdown.AcceptedObjectiveValue)
insight.Suggestion = "Good progress; monitor for regressions"

case breakdown.ObjectiveEfficiency >= 0.50:
insight.Status = "fair"
insight.Message = fmt.Sprintf("⚠️ %s: %.0f%% efficiency | %d/%d outcomes accepted | %d value delivered",
breakdown.Label,
breakdown.ObjectiveEfficiency*100,
breakdown.Accepted,
breakdown.Attempted,
breakdown.AcceptedObjectiveValue)
insight.Suggestion = "Consider reviewing agent strategy or adding human review"

default:
insight.Status = "poor"
insight.Message = fmt.Sprintf("🔴 %s: %.0f%% efficiency | %d/%d outcomes accepted | %d value delivered",
breakdown.Label,
breakdown.ObjectiveEfficiency*100,
breakdown.Accepted,
breakdown.Attempted,
breakdown.AcceptedObjectiveValue)
insight.Suggestion = "Major issues; investigate root cause or pause automation"
}

return insight
}
49 changes: 0 additions & 49 deletions pkg/parser/import_bfs.go
Original file line number Diff line number Diff line change
Expand Up @@ -380,55 +380,6 @@ func enqueueNestedImports(frontmatter map[string]any, item importQueueItem, base
return nil
}

func parseNestedImportEntries(frontmatter map[string]any) []nestedImportEntry {
importsField, hasImports := frontmatter["imports"]
if !hasImports {
return nil
}
switch v := importsField.(type) {
case []any:
nestedImports := make([]nestedImportEntry, 0, len(v))
for _, item := range v {
entry, ok := parseNestedImportEntry(item)
if !ok {
continue
}
nestedImports = append(nestedImports, entry)
}
return nestedImports
case []string:
return nestedEntriesFromSpecs(importSpecsFromStringSlice(v))
default:
return nil
}
}

func parseNestedImportEntry(item any) (nestedImportEntry, bool) {
switch nestedItem := item.(type) {
case string:
return nestedImportEntry{path: nestedItem}, true
case map[string]any:
var nestedPath string
if usesPath, ok := nestedItem["uses"].(string); ok {
nestedPath = usesPath
} else if pathVal, ok := nestedItem["path"].(string); ok {
nestedPath = pathVal
}
if nestedPath == "" {
return nestedImportEntry{}, false
}
var nestedInputs map[string]any
if withVal, ok := nestedItem["with"].(map[string]any); ok {
nestedInputs = withVal
} else if inputsVal, ok := nestedItem["inputs"].(map[string]any); ok {
nestedInputs = inputsVal
}
return nestedImportEntry{path: nestedPath, inputs: nestedInputs}, true
default:
return nestedImportEntry{}, false
}
}

func nestedEntriesFromSpecs(specs []ImportSpec) []nestedImportEntry {
nestedImports := make([]nestedImportEntry, 0, len(specs))
for _, spec := range specs {
Expand Down
19 changes: 0 additions & 19 deletions pkg/parser/import_bfs_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,25 +9,6 @@ import (
"github.com/stretchr/testify/require"
)

func TestParseNestedImportEntries_LenientArrayParsing(t *testing.T) {
frontmatter := map[string]any{
"imports": []any{
"valid-a.md",
map[string]any{"path": "valid-b.md", "inputs": map[string]any{"env": "prod"}},
map[string]any{"path": 123},
map[string]any{"inputs": map[string]any{"env": "ignored"}},
42,
},
}

entries := parseNestedImportEntries(frontmatter)
require.Len(t, entries, 2)
require.Equal(t, "valid-a.md", entries[0].path)
require.Nil(t, entries[0].inputs)
require.Equal(t, "valid-b.md", entries[1].path)
require.Equal(t, map[string]any{"env": "prod"}, entries[1].inputs)
}

func TestParseImportSpecsFromArray_RejectsIfField(t *testing.T) {
_, err := parseImportSpecsFromArray([]any{
map[string]any{
Expand Down
20 changes: 0 additions & 20 deletions pkg/workflow/compilerenv/manager.go
Original file line number Diff line number Diff line change
Expand Up @@ -46,26 +46,6 @@ const (
DefaultModelCodex = "GH_AW_DEFAULT_MODEL_CODEX"
)

// ResolveDefaultMaxDailyAICredits returns the resolved daily AI Credits guardrail
// default, checking the enterprise env var GH_AW_DEFAULT_MAX_DAILY_AI_CREDITS.
// Falls back to fallback (built-in default) when the env var is unset or invalid.
//
// A value of -1 is preserved to allow explicitly disabling the guardrail.
func ResolveDefaultMaxDailyAICredits(fallback string) string {
if raw := strings.TrimSpace(os.Getenv(DefaultMaxDailyAICredits)); raw != "" {
if raw == "-1" {
managerLog.Printf("Applying enterprise override %s=%q (fallback was %q)", DefaultMaxDailyAICredits, raw, fallback)
return "-1"
}
if normalized, ok := typeutil.NormalizeInt64KMSuffix(raw); ok {
managerLog.Printf("Applying enterprise override %s=%q (fallback was %q)", DefaultMaxDailyAICredits, normalized, fallback)
return normalized
}
managerLog.Printf("Invalid %s=%q, using fallback=%q", DefaultMaxDailyAICredits, raw, fallback)
}
return fallback
}

// ResolveDefaultMaxAICredits returns the resolved max AI credits default, checking

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

[/zoom-out] Stale spec comment in a companion file needs a follow-up cleanup.

spec_test.go line 67–68 (unchanged in this PR) still reads:

"ResolveDefaultMaxDailyAICredits is retained for other compile-time uses but MUST NOT be called for the daily guardrail default path."

Now that ResolveDefaultMaxDailyAICredits has been deleted, that sentence provides incorrect guidance to the next reader. Consider patching the spec_test.go comment in a follow-up (or squashing it into this PR) to say the function has been removed and BuildDefaultMaxDailyAICreditsExpression is the only conforming API.

💡 Suggested comment update in spec_test.go
// TestSpec_DailyAICreditsGuardrail_RuntimeNotCompileTime validates AIC spec §9.3 (2):
// GH_AW_DEFAULT_MAX_DAILY_AI_CREDITS MUST be resolved at action runtime, not at
// compiler process environment lookup. BuildDefaultMaxDailyAICreditsExpression is
// the sole conforming API for this path; the compile-time resolver
// ResolveDefaultMaxDailyAICredits has been removed as dead code.
//
// T-AIC-DG-006: The compiler produces an expression, not a pre-resolved value.

// the enterprise env var GH_AW_DEFAULT_MAX_AI_CREDITS.
// Falls back to fallback (built-in default) when the env var is unset or invalid.
Expand Down
32 changes: 0 additions & 32 deletions pkg/workflow/compilerenv/manager_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,38 +6,6 @@ import (
"github.com/stretchr/testify/assert"
)

func TestResolveDefaultMaxDailyAICredits(t *testing.T) {
t.Run("unset uses fallback", func(t *testing.T) {
t.Setenv(DefaultMaxDailyAICredits, "")
assert.Equal(t, "5000", ResolveDefaultMaxDailyAICredits("5000"))
})

t.Run("invalid uses fallback", func(t *testing.T) {
t.Setenv(DefaultMaxDailyAICredits, "abc")
assert.Equal(t, "5000", ResolveDefaultMaxDailyAICredits("5000"))
})

t.Run("zero uses fallback", func(t *testing.T) {
t.Setenv(DefaultMaxDailyAICredits, "0")
assert.Equal(t, "5000", ResolveDefaultMaxDailyAICredits("5000"))
})

t.Run("valid value overrides fallback", func(t *testing.T) {
t.Setenv(DefaultMaxDailyAICredits, "1000000")
assert.Equal(t, "1000000", ResolveDefaultMaxDailyAICredits("5000"))
})

t.Run("suffix value overrides fallback", func(t *testing.T) {
t.Setenv(DefaultMaxDailyAICredits, "2M")
assert.Equal(t, "2000000", ResolveDefaultMaxDailyAICredits("5000"))
})

t.Run("disables guardrail with -1", func(t *testing.T) {
t.Setenv(DefaultMaxDailyAICredits, "-1")
assert.Equal(t, "-1", ResolveDefaultMaxDailyAICredits("5000"))
})
}

func TestResolveDefaultMaxAICredits(t *testing.T) {
t.Run("unset uses fallback", func(t *testing.T) {
t.Setenv(DefaultMaxAICredits, "")
Expand Down
6 changes: 3 additions & 3 deletions pkg/workflow/compilerenv/spec_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -63,9 +63,9 @@ func TestSpec_DailyAICreditsGuardrail_ExpressionForm(t *testing.T) {

// TestSpec_DailyAICreditsGuardrail_RuntimeNotCompileTime validates AIC spec §9.3 (2):
// GH_AW_DEFAULT_MAX_DAILY_AI_CREDITS MUST be resolved at action runtime, not at
// compiler process environment lookup. The BuildDefaultMaxDailyAICreditsExpression
// function is the conforming API; ResolveDefaultMaxDailyAICredits is retained for
// other compile-time uses but MUST NOT be called for the daily guardrail default path.
// compiler process environment lookup. BuildDefaultMaxDailyAICreditsExpression is
// the sole conforming API for this path; the compile-time resolver
// ResolveDefaultMaxDailyAICredits has been removed as dead code.
//
// T-AIC-DG-006: The compiler produces an expression, not a pre-resolved value.
func TestSpec_DailyAICreditsGuardrail_RuntimeNotCompileTime(t *testing.T) {
Expand Down
3 changes: 0 additions & 3 deletions specs/objective-mapping-portfolio-reporting.md
Original file line number Diff line number Diff line change
Expand Up @@ -498,9 +498,6 @@ type DomainBreakdown struct {

// Aggregate outcomes by objective label
func ComputeDomainBreakdowns(reports []OutcomeReport) []DomainBreakdown

// Generate strategic insight
func AnalyzeDomainPerformance(breakdown DomainBreakdown) DomainInsight
```

## Testing
Expand Down
Loading