diff --git a/pkg/agentdrain/miner.go b/pkg/agentdrain/miner.go index 638f92738c3..efc250903a1 100644 --- a/pkg/agentdrain/miner.go +++ b/pkg/agentdrain/miner.go @@ -42,20 +42,6 @@ func NewMiner(cfg Config) (*Miner, error) { }, nil } -// Train processes a raw log line, updates the miner state, and returns the -// match result. It is safe to call from multiple goroutines. -func (m *Miner) Train(line string) (*MatchResult, error) { - masked := m.masker.Mask(line) - tokens := Tokenize(masked) - if len(tokens) == 0 { - return nil, errors.New("agentdrain: Train: empty line after masking") - } - - m.mu.Lock() - defer m.mu.Unlock() - return m.trainTokens(tokens, ""), nil -} - // trainTokens updates the miner state for tokens. Caller must hold m.mu. func (m *Miner) trainTokens(tokens []string, stage string) *MatchResult { result, _ := m.findBestMatchingCluster(tokens) diff --git a/pkg/agentdrain/miner_test.go b/pkg/agentdrain/miner_test.go index 2ff20b6ef33..dc20f174424 100644 --- a/pkg/agentdrain/miner_test.go +++ b/pkg/agentdrain/miner_test.go @@ -87,73 +87,6 @@ func TestLoadJSONRefreshesAndValidatesAnomalyThresholds(t *testing.T) { require.Error(t, m.LoadJSON(data)) } -func TestTrain(t *testing.T) { - t.Parallel() - tests := []struct { - name string - simThreshold float64 - lines []string - wantClusters int - wantWildcard bool - wantClusterIDNZ bool // last result ClusterID should be non-zero - }{ - { - name: "single line creates one cluster", - simThreshold: DefaultConfig().SimThreshold, - lines: []string{"stage=plan action=start"}, - wantClusters: 1, - wantWildcard: false, - wantClusterIDNZ: true, - }, - { - name: "two identical lines stay in one cluster without wildcard", - simThreshold: DefaultConfig().SimThreshold, - lines: []string{"stage=plan action=start", "stage=plan action=start"}, - wantClusters: 1, - wantWildcard: false, - wantClusterIDNZ: true, - }, - { - name: "two distinct lines create separate clusters", - simThreshold: DefaultConfig().SimThreshold, - lines: []string{"stage=plan action=start", "stage=finish status=ok"}, - wantClusters: 2, - wantWildcard: false, - }, - { - name: "similar lines merge and produce wildcard", - simThreshold: 0.4, - lines: []string{"stage=tool_call tool=search", "stage=tool_call tool=read_file"}, - wantClusters: 1, - wantWildcard: true, - wantClusterIDNZ: true, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - t.Parallel() - cfg := DefaultConfig() - cfg.SimThreshold = tt.simThreshold - m, err := NewMiner(cfg) - require.NoError(t, err, "NewMiner should succeed") - - var result *MatchResult - for _, line := range tt.lines { - result, err = m.Train(line) - require.NoError(t, err, "Train should not return an error for line %q", line) - } - - if tt.wantClusterIDNZ { - assert.NotZero(t, result.ClusterID, "last result ClusterID should be non-zero") - } - if tt.wantWildcard { - assert.Contains(t, result.Template, "<*>", "merged template should contain wildcard") - } - }) - } -} - func TestTrainEvent(t *testing.T) { t.Parallel() m, err := NewMiner(DefaultConfig()) @@ -176,7 +109,7 @@ func TestClusters(t *testing.T) { assert.Empty(t, m.Clusters(), "Clusters should be empty for a new miner") - _, err = m.Train("stage=plan action=start") + _, err = m.TrainEvent(AgentEvent{Stage: "plan", Fields: map[string]string{"action": "start"}}) require.NoError(t, err, "Train should not return an error") clusters := m.Clusters() @@ -335,17 +268,6 @@ func TestTokenize(t *testing.T) { } } -func TestTrainEmptyLine(t *testing.T) { - t.Parallel() - m, err := NewMiner(DefaultConfig()) - require.NoError(t, err, "NewMiner should succeed for empty-line training test") - - result, err := m.Train(" \t\n ") - assert.Nil(t, result, "Train should return nil result for whitespace-only input") - require.Error(t, err, "Train should return an error for whitespace-only input") - require.ErrorContains(t, err, "empty line after masking", "Train error should explain empty line after masking") -} - func TestNewMaskerInvalidPattern(t *testing.T) { t.Parallel() masker, err := NewMasker([]MaskRule{ @@ -374,8 +296,7 @@ func TestConcurrency(t *testing.T) { go func(id int) { defer wg.Done() for i := range linesEach { - line := fmt.Sprintf("stage=work goroutine=%d iter=%d", id, i) - _, trainErr := m.Train(line) + _, trainErr := m.TrainEvent(AgentEvent{Stage: "work", Fields: map[string]string{"goroutine": fmt.Sprintf("%d", id), "iter": fmt.Sprintf("%d", i)}}) assert.NoError(t, trainErr, "Train should not error during concurrent access") } }(g) diff --git a/pkg/agentdrain/spec_test.go b/pkg/agentdrain/spec_test.go index 63f4fa96ca6..6ac066ac9ab 100644 --- a/pkg/agentdrain/spec_test.go +++ b/pkg/agentdrain/spec_test.go @@ -506,21 +506,6 @@ func TestSpec_PublicAPI_Coordinator_LoadDefaultWeights(t *testing.T) { require.NoError(t, err, "LoadDefaultWeights should not error (no-op when empty, loads otherwise)") } -// TestSpec_PublicAPI_Miner_Train validates that Miner.Train processes a raw log line. -// Spec: "Process a raw log line (training + matching in one step)" -func TestSpec_PublicAPI_Miner_Train(t *testing.T) { - t.Parallel() - cfg := agentdrain.DefaultConfig() - miner, err := agentdrain.NewMiner(cfg) - require.NoError(t, err) - - result, err := miner.Train("user action completed step 1 successfully") - require.NoError(t, err, "Train should not error on a valid raw log line") - assert.NotNil(t, result, "Train should return a non-nil MatchResult") - assert.Positive(t, result.ClusterID, "Train result ClusterID should be positive") - assert.NotEmpty(t, result.Template, "Train result Template should be a non-empty space-joined string") -} - // TestSpec_Types_Snapshot validates the documented Snapshot/SnapshotCluster type structures. // Spec: Snapshot{Config, Clusters []SnapshotCluster, NextID}, SnapshotCluster{ID, Template, Size, Stage}. func TestSpec_Types_Snapshot(t *testing.T) { diff --git a/pkg/intent/governance.go b/pkg/intent/governance.go index a399b6fa954..0f069cab129 100644 --- a/pkg/intent/governance.go +++ b/pkg/intent/governance.go @@ -18,45 +18,6 @@ var ErrToolDenied = errors.New("intent: tool denied by policy") // AllowedTools is non-nil (restricted) and does not contain the requested tool. var ErrToolNotAllowed = errors.New("intent: tool not allowed by policy") -// ResolveRisk returns rec.Risk when explicitly set; otherwise it derives a risk -// classification from rec.Domains and rec.Priority using deterministic, -// precedence-ordered rules: -// -// security + critical priority -> high -// production -> high -// infrastructure -> medium -// documentation -> low -// anything else -> unknown -// -// An explicit Risk always wins over any derived value, even when the record's -// domains or priority would otherwise match a different rule. -func ResolveRisk(rec IntentRecord) string { - if rec.Risk != "" { - governanceLog.Printf("ResolveRisk: using explicit risk=%s", rec.Risk) - return rec.Risk - } - - if slices.Contains(rec.Domains, "security") && rec.Priority == "critical" { - governanceLog.Print("ResolveRisk: security+critical -> high") - return "high" - } - if slices.Contains(rec.Domains, "production") { - governanceLog.Print("ResolveRisk: production -> high") - return "high" - } - if slices.Contains(rec.Domains, "infrastructure") { - governanceLog.Print("ResolveRisk: infrastructure -> medium") - return "medium" - } - if slices.Contains(rec.Domains, "documentation") { - governanceLog.Print("ResolveRisk: documentation -> low") - return "low" - } - - governanceLog.Print("ResolveRisk: no matching rule -> unknown") - return "unknown" -} - // Authorizer authorizes individual tool calls against a compiled ExecutionPolicy. type Authorizer struct{} diff --git a/pkg/intent/governance_formal_test.go b/pkg/intent/governance_formal_test.go index 52385c0a3f6..16e2cb0739b 100644 --- a/pkg/intent/governance_formal_test.go +++ b/pkg/intent/governance_formal_test.go @@ -12,98 +12,9 @@ import ( ) // Formal test suite derived from specs/intent-attribution-agent-governance.md, -// focusing on the Risk classification (ResolveRisk) and Enforcement -// (Authorizer.AuthorizeTool) sections, plus fail-closed policy compilation for -// unlinked/ambiguous attribution. Each test corresponds to a named predicate or -// invariant in the behavioral coverage map. - -// TestResolveRisk_ExplicitOverride (P1/P2 — RiskExplicitOverride) -// Invariant: an explicit intent.Risk always wins over derived rules, even with -// conflicting domains/priority that would otherwise resolve differently. -func TestResolveRisk_ExplicitOverride(t *testing.T) { - rec := intent.IntentRecord{ - Risk: "low", - Domains: []string{"security", "production"}, - Priority: "critical", - } - assert.Equal(t, "low", intent.ResolveRisk(rec), - "P1/P2: explicit risk must win over derived rules") -} - -// TestResolveRisk_SecurityCriticalIsHigh (P3 — RiskSecurityCriticalHigh) -// Invariant: domains contains security AND priority == critical => high. -func TestResolveRisk_SecurityCriticalIsHigh(t *testing.T) { - rec := intent.IntentRecord{ - Domains: []string{"security"}, - Priority: "critical", - } - assert.Equal(t, "high", intent.ResolveRisk(rec), - "P3: security+critical must resolve to high") -} - -// TestResolveRisk_ProductionIsHigh (P4 — RiskProductionHigh) -// Invariant: domains contains production => high, independent of priority. -func TestResolveRisk_ProductionIsHigh(t *testing.T) { - cases := []string{"", "low", "critical", "unrecognized"} - for _, priority := range cases { - t.Run("priority="+priority, func(t *testing.T) { - rec := intent.IntentRecord{ - Domains: []string{"production"}, - Priority: priority, - } - assert.Equal(t, "high", intent.ResolveRisk(rec), - "P4: production domain must resolve to high regardless of priority") - }) - } -} - -// TestResolveRisk_InfrastructureIsMedium (P5 — RiskInfrastructureMedium) -// Invariant: domains contains infrastructure => medium. -func TestResolveRisk_InfrastructureIsMedium(t *testing.T) { - rec := intent.IntentRecord{Domains: []string{"infrastructure"}} - assert.Equal(t, "medium", intent.ResolveRisk(rec), - "P5: infrastructure domain must resolve to medium") -} - -// TestResolveRisk_DocumentationIsLow (P6 — RiskDocumentationLow) -// Invariant: domains contains documentation => low. -func TestResolveRisk_DocumentationIsLow(t *testing.T) { - rec := intent.IntentRecord{Domains: []string{"documentation"}} - assert.Equal(t, "low", intent.ResolveRisk(rec), - "P6: documentation domain must resolve to low") -} - -// TestResolveRisk_UnknownDefault (P7 — RiskUnknownDefault) -// Invariant: no matching rule (empty, unrecognized domain, security without -// critical priority) => unknown. -func TestResolveRisk_UnknownDefault(t *testing.T) { - cases := []struct { - name string - rec intent.IntentRecord - }{ - {"empty", intent.IntentRecord{}}, - {"unrecognized_domain", intent.IntentRecord{Domains: []string{"marketing"}}}, - {"security_without_critical", intent.IntentRecord{Domains: []string{"security"}, Priority: "low"}}, - {"security_no_priority", intent.IntentRecord{Domains: []string{"security"}}}, - } - for _, tc := range cases { - t.Run(tc.name, func(t *testing.T) { - assert.Equal(t, "unknown", intent.ResolveRisk(tc.rec), - "P7: non-matching input must resolve to unknown") - }) - } -} - -// TestResolveRisk_PrecedenceOrder (P8 — RiskPrecedenceOrder) -// Invariant: security+critical takes precedence when multiple domains overlap. -func TestResolveRisk_PrecedenceOrder(t *testing.T) { - rec := intent.IntentRecord{ - Domains: []string{"documentation", "infrastructure", "production", "security"}, - Priority: "critical", - } - assert.Equal(t, "high", intent.ResolveRisk(rec), - "P8: security+critical must take precedence over other overlapping domains") -} +// focusing on Enforcement (Authorizer.AuthorizeTool) sections, plus fail-closed +// policy compilation for unlinked/ambiguous attribution. Each test corresponds +// to a named predicate or invariant in the behavioral coverage map. // TestAuthorizeTool_DeniedWins (P9 — AuthorizeToolDeniedWins) // Invariant: a tool in DeniedTools is rejected even if it also appears in @@ -187,14 +98,6 @@ func TestSafestDefaultPolicy_FailClosedForIndeterminateStatus(t *testing.T) { } } -// TestEdgeCase_EmptyDomainsAndPriority validates that a fully empty intent -// record resolves to unknown, not a panic or empty string. -func TestEdgeCase_EmptyDomainsAndPriority(t *testing.T) { - risk := intent.ResolveRisk(intent.IntentRecord{}) - assert.Equal(t, "unknown", risk, "edge case: fully empty record must resolve to unknown") - assert.NotEmpty(t, risk, "edge case: ResolveRisk must never return an empty string") -} - // TestEdgeCase_NilDeniedAndAllowedTools validates that AuthorizeTool does not // panic on a zero-value policy. func TestEdgeCase_NilDeniedAndAllowedTools(t *testing.T) { diff --git a/pkg/scanfindings/scanfindings.go b/pkg/scanfindings/scanfindings.go index 7c6a68ecd4b..5d91865bad5 100644 --- a/pkg/scanfindings/scanfindings.go +++ b/pkg/scanfindings/scanfindings.go @@ -11,10 +11,8 @@ package scanfindings import ( - "cmp" "fmt" "io" - "slices" "strings" "github.com/github/gh-aw/pkg/console" @@ -169,38 +167,6 @@ func Render(w io.Writer, findings []Finding) { } } -// Sort orders findings by file, then line, then column, then by decreasing -// severity, then by rule identifier. The ordering is stable and deterministic so -// that scanner output can be compared across runs. -func Sort(findings []Finding) { - slices.SortStableFunc(findings, func(a, b Finding) int { - if c := strings.Compare(a.File, b.File); c != 0 { - return c - } - if c := cmp.Compare(a.Line, b.Line); c != 0 { - return c - } - if c := cmp.Compare(a.Column, b.Column); c != 0 { - return c - } - if c := cmp.Compare(b.Severity.Rank(), a.Severity.Rank()); c != 0 { - return c - } - return strings.Compare(a.RuleID, b.RuleID) - }) -} - -// CountAtLeast returns the number of findings with a severity of at least min. -func CountAtLeast(findings []Finding, min SeverityLevel) int { - count := 0 - for _, finding := range findings { - if finding.Severity.AtLeast(min) { - count++ - } - } - return count -} - // ContextLines returns a symmetric window of up to two source lines before and // after the 1-based line number. The window shrinks at file boundaries to keep // the target line at its midpoint for context rendering. It returns nil when diff --git a/pkg/scanfindings/scanfindings_test.go b/pkg/scanfindings/scanfindings_test.go index 5e8483e7b9c..b19e53bbb20 100644 --- a/pkg/scanfindings/scanfindings_test.go +++ b/pkg/scanfindings/scanfindings_test.go @@ -118,65 +118,6 @@ func TestFormatMessage(t *testing.T) { } } -func TestSort(t *testing.T) { - findings := []Finding{ - {File: "b.yml", Line: 1, Severity: SeverityLow}, - {File: "a.yml", Line: 10, Column: 2, Severity: SeverityMedium}, - {File: "a.yml", Line: 10, Column: 1, Severity: SeverityLow}, - {File: "a.yml", Line: 2, Severity: SeverityInfo}, - } - - Sort(findings) - - want := []struct { - file string - line int - column int - }{ - {"a.yml", 2, 0}, - {"a.yml", 10, 1}, - {"a.yml", 10, 2}, - {"b.yml", 1, 0}, - } - - for i, w := range want { - got := findings[i] - if got.File != w.file || got.Line != w.line || got.Column != w.column { - t.Errorf("finding %d = %s:%d:%d, want %s:%d:%d", i, got.File, got.Line, got.Column, w.file, w.line, w.column) - } - } -} - -func TestSortOrdersBySeverityWithinSameLocation(t *testing.T) { - findings := []Finding{ - {File: "a.yml", Line: 1, Column: 1, Severity: SeverityLow, RuleID: "low"}, - {File: "a.yml", Line: 1, Column: 1, Severity: SeverityCritical, RuleID: "critical"}, - } - - Sort(findings) - - if findings[0].RuleID != "critical" { - t.Errorf("expected critical finding first, got %q", findings[0].RuleID) - } -} - -func TestCountAtLeast(t *testing.T) { - findings := []Finding{ - {Severity: SeverityCritical}, - {Severity: SeverityHigh}, - {Severity: SeverityMedium}, - {Severity: SeverityUnknown}, - } - - if got := CountAtLeast(findings, SeverityHigh); got != 2 { - t.Errorf("CountAtLeast(high) = %d, want 2", got) - } - // Unknown severities rank below info and are therefore excluded. - if got := CountAtLeast(findings, SeverityInfo); got != 3 { - t.Errorf("CountAtLeast(info) = %d, want 3", got) - } -} - func TestContextLines(t *testing.T) { lines := []string{"one", "two", "three", "four", "five", "six"} diff --git a/pkg/workflow/mcp_gateway_mount_policy_test.go b/pkg/workflow/mcp_gateway_mount_policy_test.go index adabf58cf72..ccd19dda2fd 100644 --- a/pkg/workflow/mcp_gateway_mount_policy_test.go +++ b/pkg/workflow/mcp_gateway_mount_policy_test.go @@ -143,54 +143,6 @@ func TestMCPGatewayContainerCommandIncludesAllowedMountRootsEnvFlag(t *testing.T assert.Contains(t, containerCmd.String(), " -e RUNNER_TOOL_CACHE") } -func TestMcpGatewayMountsUseRunnerToolCache(t *testing.T) { - tests := []struct { - name string - tools map[string]any - gatewayConfig *MCPGatewayRuntimeConfig - expected bool - }{ - { - name: "escaped tool mount", - tools: map[string]any{ - "serena": map[string]any{ - "mounts": []any{`\${RUNNER_TOOL_CACHE}:\${RUNNER_TOOL_CACHE}:ro`}, - }, - }, - expected: true, - }, - { - name: "unescaped tool mount", - tools: map[string]any{ - "serena": map[string]any{ - "mounts": []any{`${RUNNER_TOOL_CACHE}:${RUNNER_TOOL_CACHE}:ro`}, - }, - }, - expected: true, - }, - { - name: "gateway mount", - gatewayConfig: &MCPGatewayRuntimeConfig{Mounts: []string{`${RUNNER_TOOL_CACHE}:${RUNNER_TOOL_CACHE}:ro`}}, - expected: true, - }, - { - name: "unrelated mount", - tools: map[string]any{ - "server": map[string]any{ - "mounts": []any{`${GITHUB_WORKSPACE}:${GITHUB_WORKSPACE}:rw`}, - }, - }, - expected: false, - }, - } - - for _, test := range tests { - t.Run(test.name, func(t *testing.T) { - assert.Equal(t, test.expected, mcpGatewayMountsUseRunnerToolCache(test.tools, test.gatewayConfig)) - }) - } -} - // TestWriteMCPGatewayExportsIncludesAllowedMountRoots verifies the run script // exports MCP_GATEWAY_ALLOWED_MOUNT_ROOTS before starting the gateway. func TestWriteMCPGatewayExportsIncludesAllowedMountRoots(t *testing.T) { diff --git a/pkg/workflow/mcp_setup_gateway.go b/pkg/workflow/mcp_setup_gateway.go index 1ead177288d..9fa0e7cd18b 100644 --- a/pkg/workflow/mcp_setup_gateway.go +++ b/pkg/workflow/mcp_setup_gateway.go @@ -568,19 +568,6 @@ func collectMCPServerConfiguredMounts(tools map[string]any) []string { return mounts } -func mcpGatewayMountsUseRunnerToolCache(tools map[string]any, gatewayConfig *MCPGatewayRuntimeConfig) bool { - mounts := collectMCPServerConfiguredMounts(tools) - if gatewayConfig != nil { - mounts = append(mounts, gatewayConfig.Mounts...) - } - for _, mount := range mounts { - if strings.Contains(strings.ReplaceAll(mount, `\$`, "$"), "${RUNNER_TOOL_CACHE}") { - return true - } - } - return false -} - // extractMCPMountsField normalizes a raw "mounts" field value (as produced by // YAML/JSON frontmatter parsing) into a slice of "source:dest[:mode]" strings. func extractMCPMountsField(mountsRaw any) []string {