diff --git a/pkg/cli/compile_guard_policy_report.go b/pkg/cli/compile_guard_policy_report.go new file mode 100644 index 00000000000..df0065c082d --- /dev/null +++ b/pkg/cli/compile_guard_policy_report.go @@ -0,0 +1,149 @@ +package cli + +import ( + "fmt" + "os" + "sort" + "strings" + + "github.com/github/gh-aw/pkg/console" + "github.com/github/gh-aw/pkg/workflow" +) + +// guardPolicyDryRunReport summarizes the effective GitHub guard-policy +// configuration for a single compiled workflow. It is produced in --strict +// mode as a compile-time dry-run of which repositories the guard policy +// would permit or deny, addressing Open Question #4 in +// scratchpad/guard-policies-specification.md ("Should we add a 'dry-run' +// mode to test policies before enforcement?"). +type guardPolicyDryRunReport struct { + Workflow string + Lockdown bool + PermittedRepos string + MinIntegrity string + BlockedUsers []string + TrustedUsers []string + ApprovalLabels []string +} + +// hasGuardPolicyFields reports whether the GitHub tool config has any +// guard-policy fields configured (allowed-repos, min-integrity, blocked-users, +// trusted-users, approval-labels). +func hasGuardPolicyFields(github *workflow.GitHubToolConfig) bool { + if github == nil { + return false + } + hasRepos := github.AllowedRepos != nil || github.Repos != nil + hasMinIntegrity := github.MinIntegrity != "" + hasBlockedUsers := len(github.BlockedUsers) > 0 || github.BlockedUsersExpr != "" + hasApprovalLabels := len(github.ApprovalLabels) > 0 || github.ApprovalLabelsExpr != "" + hasTrustedUsers := len(github.TrustedUsers) > 0 || github.TrustedUsersExpr != "" + return hasRepos || hasMinIntegrity || hasBlockedUsers || hasApprovalLabels || hasTrustedUsers +} + +// formatGuardPolicyReposScope renders a GitHubReposScope value ("all", +// "public", or an array of repository patterns) as a human-readable string +// for the dry-run report. +func formatGuardPolicyReposScope(scope workflow.GitHubReposScope) string { + switch v := scope.(type) { + case nil: + return "all (default)" + case string: + if v == "" { + return "all (default)" + } + return v + case []any: + patterns := make([]string, 0, len(v)) + for _, item := range v { + if s, ok := item.(string); ok && s != "" { + patterns = append(patterns, s) + } + } + sort.Strings(patterns) + if len(patterns) == 0 { + return "all (default)" + } + return strings.Join(patterns, ", ") + case []string: + patterns := append([]string(nil), v...) + sort.Strings(patterns) + if len(patterns) == 0 { + return "all (default)" + } + return strings.Join(patterns, ", ") + default: + return fmt.Sprintf("%v", v) + } +} + +// buildGuardPolicyDryRunReport builds a guardPolicyDryRunReport for a +// workflow's GitHub tool configuration. Returns nil when no guard-policy +// fields are configured (nothing to report). +func buildGuardPolicyDryRunReport(workflowName string, github *workflow.GitHubToolConfig) *guardPolicyDryRunReport { + if github == nil || !hasGuardPolicyFields(github) { + return nil + } + + repos := github.AllowedRepos + if repos == nil { + repos = github.Repos + } + + minIntegrity := string(github.MinIntegrity) + if minIntegrity == "" { + minIntegrity = "none (default)" + } + + return &guardPolicyDryRunReport{ + Workflow: workflowName, + Lockdown: github.Lockdown, + PermittedRepos: formatGuardPolicyReposScope(repos), + MinIntegrity: minIntegrity, + BlockedUsers: github.BlockedUsers, + TrustedUsers: github.TrustedUsers, + ApprovalLabels: github.ApprovalLabels, + } +} + +// formatGuardPolicyDryRunReport renders a guardPolicyDryRunReport as a +// human-readable multi-line string for --strict compile output. +func formatGuardPolicyDryRunReport(report *guardPolicyDryRunReport) string { + if report == nil { + return "" + } + + var b strings.Builder + fmt.Fprintf(&b, "guard policy dry-run report for %s:\n", report.Workflow) + if report.Lockdown { + b.WriteString(" lockdown: true (guard-policy fields below are not evaluated at runtime)\n") + } + fmt.Fprintf(&b, " allowed-repos: %s\n", report.PermittedRepos) + fmt.Fprintf(&b, " min-integrity: %s\n", report.MinIntegrity) + if len(report.BlockedUsers) > 0 { + fmt.Fprintf(&b, " blocked-users: %s\n", strings.Join(report.BlockedUsers, ", ")) + } + if len(report.TrustedUsers) > 0 { + fmt.Fprintf(&b, " trusted-users: %s\n", strings.Join(report.TrustedUsers, ", ")) + } + if len(report.ApprovalLabels) > 0 { + fmt.Fprintf(&b, " approval-labels: %s\n", strings.Join(report.ApprovalLabels, ", ")) + } + return strings.TrimRight(b.String(), "\n") +} + +// printGuardPolicyDryRunReport prints a compile-time guard-policy dry-run +// report to stderr when --strict is set and the workflow has a GitHub guard +// policy configured. It is a no-op otherwise. +func printGuardPolicyDryRunReport(workflowName string, workflowData *workflow.WorkflowData, strict bool) { + if !strict || workflowData == nil || workflowData.ParsedTools == nil { + return + } + + report := buildGuardPolicyDryRunReport(workflowName, workflowData.ParsedTools.GitHub) + if report == nil { + return + } + + fmt.Fprintln(os.Stderr, console.FormatInfoMessageStderr(formatGuardPolicyDryRunReport(report))) +} diff --git a/pkg/cli/compile_guard_policy_report_test.go b/pkg/cli/compile_guard_policy_report_test.go new file mode 100644 index 00000000000..5786247ec6c --- /dev/null +++ b/pkg/cli/compile_guard_policy_report_test.go @@ -0,0 +1,142 @@ +//go:build !integration + +package cli + +import ( + "io" + "os" + "testing" + + "github.com/github/gh-aw/pkg/workflow" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// captureStderrForGuardPolicyReportTest captures stderr output produced while +// running fn, for use by the guard-policy dry-run report tests in this file. +func captureStderrForGuardPolicyReportTest(fn func()) string { + old := os.Stderr + r, w, err := os.Pipe() + if err != nil { + panic(err) + } + os.Stderr = w + + fn() + + if err := w.Close(); err != nil { + panic(err) + } + os.Stderr = old + + out, err := io.ReadAll(r) + if err != nil { + panic(err) + } + return string(out) +} + +// TestBuildGuardPolicyDryRunReport_NoGuardPolicy verifies that no report is +// produced when no guard-policy fields are configured. +func TestBuildGuardPolicyDryRunReport_NoGuardPolicy(t *testing.T) { + assert.Nil(t, buildGuardPolicyDryRunReport("test.md", nil)) + assert.Nil(t, buildGuardPolicyDryRunReport("test.md", &workflow.GitHubToolConfig{})) +} + +// TestBuildGuardPolicyDryRunReport_AllowedReposAndMinIntegrity verifies that a +// dry-run report is produced summarizing allowed-repos and min-integrity. +func TestBuildGuardPolicyDryRunReport_AllowedReposAndMinIntegrity(t *testing.T) { + github := &workflow.GitHubToolConfig{ + AllowedRepos: []any{"owner/repo-b", "owner/repo-a"}, + MinIntegrity: workflow.GitHubIntegrityApproved, + } + + report := buildGuardPolicyDryRunReport("test-workflow.md", github) + require.NotNil(t, report) + assert.Equal(t, "test-workflow.md", report.Workflow) + assert.False(t, report.Lockdown) + assert.Equal(t, "owner/repo-a, owner/repo-b", report.PermittedRepos) + assert.Equal(t, "approved", report.MinIntegrity) +} + +// TestBuildGuardPolicyDryRunReport_AllScope verifies that an "all" scope +// (or omitted allowed-repos) is reported as such. +func TestBuildGuardPolicyDryRunReport_AllScope(t *testing.T) { + github := &workflow.GitHubToolConfig{ + MinIntegrity: workflow.GitHubIntegrityNone, + } + + report := buildGuardPolicyDryRunReport("test-workflow.md", github) + require.NotNil(t, report) + assert.Equal(t, "all (default)", report.PermittedRepos) + assert.Equal(t, "none", report.MinIntegrity) +} + +// TestBuildGuardPolicyDryRunReport_BlockedTrustedApproval verifies that +// blocked-users, trusted-users, and approval-labels are surfaced in the report. +func TestBuildGuardPolicyDryRunReport_BlockedTrustedApproval(t *testing.T) { + github := &workflow.GitHubToolConfig{ + MinIntegrity: workflow.GitHubIntegrityApproved, + BlockedUsers: []string{"spam-bot"}, + TrustedUsers: []string{"trusted-user"}, + ApprovalLabels: []string{"human-reviewed"}, + } + + report := buildGuardPolicyDryRunReport("test-workflow.md", github) + require.NotNil(t, report) + assert.Equal(t, []string{"spam-bot"}, report.BlockedUsers) + assert.Equal(t, []string{"trusted-user"}, report.TrustedUsers) + assert.Equal(t, []string{"human-reviewed"}, report.ApprovalLabels) + + rendered := formatGuardPolicyDryRunReport(report) + assert.Contains(t, rendered, "blocked-users: spam-bot") + assert.Contains(t, rendered, "trusted-users: trusted-user") + assert.Contains(t, rendered, "approval-labels: human-reviewed") +} + +// TestBuildGuardPolicyDryRunReport_Lockdown verifies that lockdown is +// surfaced in the report to make clear the guard-policy fields are ignored +// at runtime per §9.5 of scratchpad/github-mcp-access-control-specification.md. +func TestBuildGuardPolicyDryRunReport_Lockdown(t *testing.T) { + github := &workflow.GitHubToolConfig{ + Lockdown: true, + AllowedRepos: "all", + MinIntegrity: workflow.GitHubIntegrityApproved, + } + + report := buildGuardPolicyDryRunReport("test-workflow.md", github) + require.NotNil(t, report) + assert.True(t, report.Lockdown) + + rendered := formatGuardPolicyDryRunReport(report) + assert.Contains(t, rendered, "lockdown: true") +} + +// TestPrintGuardPolicyDryRunReport_OnlyWhenStrict verifies that the dry-run +// report is only emitted when --strict is set, and only when guard-policy +// fields are configured. +func TestPrintGuardPolicyDryRunReport_OnlyWhenStrict(t *testing.T) { + workflowData := &workflow.WorkflowData{ + ParsedTools: &workflow.Tools{ + GitHub: &workflow.GitHubToolConfig{ + MinIntegrity: workflow.GitHubIntegrityApproved, + }, + }, + } + + stderrOutput := captureStderrForGuardPolicyReportTest(func() { + printGuardPolicyDryRunReport("test-workflow.md", workflowData, false) + }) + assert.Empty(t, stderrOutput, "no report should be emitted when --strict is not set") + + stderrOutput = captureStderrForGuardPolicyReportTest(func() { + printGuardPolicyDryRunReport("test-workflow.md", workflowData, true) + }) + assert.Contains(t, stderrOutput, "guard policy dry-run report for test-workflow.md") + + noGuardPolicyData := &workflow.WorkflowData{ParsedTools: &workflow.Tools{}} + stderrOutput = captureStderrForGuardPolicyReportTest(func() { + printGuardPolicyDryRunReport("test-workflow.md", noGuardPolicyData, true) + }) + assert.Empty(t, stderrOutput, "no report should be emitted without guard-policy fields") +} diff --git a/pkg/cli/compile_workflow_processor.go b/pkg/cli/compile_workflow_processor.go index 207487bb092..9028583be0a 100644 --- a/pkg/cli/compile_workflow_processor.go +++ b/pkg/cli/compile_workflow_processor.go @@ -185,6 +185,11 @@ func compileWorkflowFile( // Collect labels for JSON output (used by create-labels maintenance operation) result.validationResult.Labels = extractSafeOutputLabels(workflowData) + // Emit a compile-time guard-policy dry-run report in --strict mode. + if !opts.jsonOutput { + printGuardPolicyDryRunReport(filepath.Base(resolvedFile), workflowData, opts.strict) + } + compileWorkflowProcessorLog.Printf("Successfully processed workflow file: %s", resolvedFile) return result } diff --git a/pkg/workflow/awf_config_drift_test.go b/pkg/workflow/awf_config_drift_test.go new file mode 100644 index 00000000000..881326be380 --- /dev/null +++ b/pkg/workflow/awf_config_drift_test.go @@ -0,0 +1,255 @@ +//go:build !integration + +package workflow + +import ( + "bytes" + "encoding/json" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// driftRecord mirrors the DriftRecord entity schema defined in +// specs/awf-config-sources-spec.md §6.5.1 and validated by the conformance +// test IDs listed in specs/awf-config-sources-compliance/README.md. +type driftRecord struct { + PropertyPath string `json:"property_path"` + DriftCategory string `json:"drift_category"` + SuggestedAction string `json:"suggested_action"` + DetectedAt string `json:"detected_at"` +} + +// driftCategories enumerates the valid drift_category values per §6.5.1. +var driftCategories = map[string]bool{ + "missing_in_ghaw": true, + "missing_in_schema": true, + "spec_mismatch": true, +} + +// validateDriftRecord validates a driftRecord against the §6.5.1 requirements: +// required fields present, drift_category in the allowed enum, detected_at is a +// valid ISO 8601 UTC timestamp, and suggested_action is non-empty. +func validateDriftRecord(r driftRecord) error { + if r.PropertyPath == "" || r.DriftCategory == "" || r.SuggestedAction == "" || r.DetectedAt == "" { + return assertError("DriftRecord is missing one or more required fields (property_path, drift_category, suggested_action, detected_at)") + } + if !driftCategories[r.DriftCategory] { + return assertError("drift_category must be one of missing_in_ghaw, missing_in_schema, spec_mismatch; got " + r.DriftCategory) + } + if _, err := time.Parse(time.RFC3339, r.DetectedAt); err != nil { + return assertError("detected_at must be a valid ISO 8601 UTC timestamp: " + err.Error()) + } + return nil +} + +type assertError string + +func (e assertError) Error() string { return string(e) } + +// validateDriftRecordJSONStrict decodes raw JSON into a driftRecord while +// rejecting any properties beyond the four required fields, per §6.5.1 +// "no additional properties". +func validateDriftRecordJSONStrict(raw []byte) (driftRecord, error) { + var r driftRecord + dec := json.NewDecoder(bytes.NewReader(raw)) + dec.DisallowUnknownFields() + if err := dec.Decode(&r); err != nil { + return driftRecord{}, err + } + return r, nil +} + +// driftRequiresCorrectivePR implements §6.5.3: a corrective PR MUST be opened +// when any DriftRecord in the list has an actionable drift_category. +func driftRequiresCorrectivePR(records []driftRecord) bool { + for _, r := range records { + if r.DriftCategory == "missing_in_ghaw" || r.DriftCategory == "spec_mismatch" { + return true + } + } + return false +} + +// driftRequiresSLAEscalation implements §6.5.3: an escalation issue MUST be +// opened or updated when the SLA window has been exceeded and actionable +// DriftRecord items are present. +func driftRequiresSLAEscalation(records []driftRecord, slaExceeded bool) bool { + return slaExceeded && driftRequiresCorrectivePR(records) +} + +// driftCorrectivePRBody implements §6.5.3: the corrective PR description MUST +// embed the full DriftRecord list as JSON. +func driftCorrectivePRBody(records []driftRecord) (string, error) { + b, err := json.Marshal(records) + if err != nil { + return "", err + } + return string(b), nil +} + +// TestDriftRecord_TDR001_RequiredFields validates T-DR-001: DriftRecord MUST +// include property_path, drift_category, suggested_action, and detected_at; +// records missing any required field are invalid and MUST be rejected. +func TestDriftRecord_TDR001_RequiredFields(t *testing.T) { + valid := driftRecord{ + PropertyPath: "apiProxy.anthropicAutoCache", + DriftCategory: "missing_in_ghaw", + SuggestedAction: "Add coverage", + DetectedAt: "2026-06-08T00:00:00Z", + } + require.NoError(t, validateDriftRecord(valid)) + + missingFields := []driftRecord{ + {DriftCategory: "missing_in_ghaw", SuggestedAction: "Add coverage", DetectedAt: "2026-06-08T00:00:00Z"}, + {PropertyPath: "x", SuggestedAction: "Add coverage", DetectedAt: "2026-06-08T00:00:00Z"}, + {PropertyPath: "x", DriftCategory: "missing_in_ghaw", DetectedAt: "2026-06-08T00:00:00Z"}, + {PropertyPath: "x", DriftCategory: "missing_in_ghaw", SuggestedAction: "Add coverage"}, + } + for _, r := range missingFields { + assert.Error(t, validateDriftRecord(r)) + } +} + +// TestDriftRecord_TDR002_DriftCategoryEnum validates T-DR-002: drift_category +// MUST be one of missing_in_ghaw, missing_in_schema, or spec_mismatch; any +// other value is invalid. +func TestDriftRecord_TDR002_DriftCategoryEnum(t *testing.T) { + base := driftRecord{PropertyPath: "x", SuggestedAction: "y", DetectedAt: "2026-06-08T00:00:00Z"} + + for _, category := range []string{"missing_in_ghaw", "missing_in_schema", "spec_mismatch"} { + r := base + r.DriftCategory = category + require.NoError(t, validateDriftRecord(r)) + } + + invalid := base + invalid.DriftCategory = "unknown_category" + assert.Error(t, validateDriftRecord(invalid)) +} + +// TestDriftRecord_TDR003_DetectedAtFormat validates T-DR-003: detected_at MUST +// be a valid ISO 8601 UTC timestamp; non-conforming values MUST be rejected. +func TestDriftRecord_TDR003_DetectedAtFormat(t *testing.T) { + base := driftRecord{PropertyPath: "x", DriftCategory: "missing_in_ghaw", SuggestedAction: "y"} + + valid := base + valid.DetectedAt = "2026-06-08T00:00:00Z" + require.NoError(t, validateDriftRecord(valid)) + + for _, ts := range []string{"not-a-timestamp", "2026-06-08", "06/08/2026"} { + invalid := base + invalid.DetectedAt = ts + assert.Error(t, validateDriftRecord(invalid)) + } +} + +// TestDriftRecord_TDR004_SuggestedActionNonEmpty validates T-DR-004: +// suggested_action MUST NOT be empty; an empty string MUST be rejected. +func TestDriftRecord_TDR004_SuggestedActionNonEmpty(t *testing.T) { + invalid := driftRecord{ + PropertyPath: "x", + DriftCategory: "missing_in_ghaw", + DetectedAt: "2026-06-08T00:00:00Z", + } + assert.Error(t, validateDriftRecord(invalid)) +} + +// TestDriftRecord_TDR005_NoAdditionalProperties validates T-DR-005: +// DriftRecord objects MUST NOT include properties beyond the four required +// fields; additional properties MUST be rejected. +func TestDriftRecord_TDR005_NoAdditionalProperties(t *testing.T) { + validJSON := []byte(`{"property_path":"x","drift_category":"missing_in_ghaw","suggested_action":"y","detected_at":"2026-06-08T00:00:00Z"}`) + r, err := validateDriftRecordJSONStrict(validJSON) + require.NoError(t, err) + assert.Equal(t, "x", r.PropertyPath) + + withExtra := []byte(`{"property_path":"x","drift_category":"missing_in_ghaw","suggested_action":"y","detected_at":"2026-06-08T00:00:00Z","extra_field":"nope"}`) + _, err = validateDriftRecordJSONStrict(withExtra) + assert.Error(t, err) +} + +// TestDriftRecord_TDR006_CorrectivePRTrigger validates T-DR-006: when any +// DriftRecord in the output list has drift_category of missing_in_ghaw or +// spec_mismatch, the detecting automation MUST open a corrective PR (CR-05). +func TestDriftRecord_TDR006_CorrectivePRTrigger(t *testing.T) { + assert.True(t, driftRequiresCorrectivePR([]driftRecord{{DriftCategory: "missing_in_ghaw"}})) + assert.True(t, driftRequiresCorrectivePR([]driftRecord{{DriftCategory: "spec_mismatch"}})) + assert.False(t, driftRequiresCorrectivePR([]driftRecord{{DriftCategory: "missing_in_schema"}})) + assert.False(t, driftRequiresCorrectivePR(nil)) +} + +// TestDriftRecord_TDR007_SLAEscalationTrigger validates T-DR-007: when the +// CR-06 SLA window is exceeded and DriftRecord items with actionable +// categories are present, an escalation issue MUST be opened or updated. +func TestDriftRecord_TDR007_SLAEscalationTrigger(t *testing.T) { + actionable := []driftRecord{{DriftCategory: "missing_in_ghaw"}} + nonActionable := []driftRecord{{DriftCategory: "missing_in_schema"}} + + assert.True(t, driftRequiresSLAEscalation(actionable, true)) + assert.False(t, driftRequiresSLAEscalation(actionable, false), "escalation must not fire before the SLA window is exceeded") + assert.False(t, driftRequiresSLAEscalation(nonActionable, true), "escalation must not fire without actionable drift") +} + +// TestDriftRecord_TDR008_CorrectivePREmbedsRecords validates T-DR-008: the +// corrective PR description MUST embed the full DriftRecord list as JSON. +func TestDriftRecord_TDR008_CorrectivePREmbedsRecords(t *testing.T) { + records := []driftRecord{ + { + PropertyPath: "apiProxy.anthropicAutoCache", + DriftCategory: "missing_in_ghaw", + SuggestedAction: "Add coverage", + DetectedAt: "2026-06-08T00:00:00Z", + }, + } + + body, err := driftCorrectivePRBody(records) + require.NoError(t, err) + + var decoded []driftRecord + require.NoError(t, json.Unmarshal([]byte(body), &decoded)) + assert.Equal(t, records, decoded) +} + +// TestDriftRecord_TDR009_EmptyListValid validates T-DR-009: an empty +// DriftRecord list (no drift detected) is a valid output and MUST NOT +// trigger corrective PR or escalation actions. +func TestDriftRecord_TDR009_EmptyListValid(t *testing.T) { + assert.False(t, driftRequiresCorrectivePR([]driftRecord{})) + assert.False(t, driftRequiresSLAEscalation([]driftRecord{}, true)) + + body, err := driftCorrectivePRBody([]driftRecord{}) + require.NoError(t, err) + assert.Equal(t, "[]", body) +} + +// TestDriftRecord_TDR010_Step5Integration validates T-DR-010: the drift +// detection procedure Step 5 MUST produce a list of zero or more DriftRecord +// objects; the output format MUST be a JSON array conforming to the §6.5.1 +// schema. +func TestDriftRecord_TDR010_Step5Integration(t *testing.T) { + records := []driftRecord{ + { + PropertyPath: "container.dockerHostPathPrefix", + DriftCategory: "spec_mismatch", + SuggestedAction: "Reconcile implementation with spec", + DetectedAt: "2026-06-08T00:00:00Z", + }, + } + + body, err := driftCorrectivePRBody(records) + require.NoError(t, err) + + var decoded []driftRecord + require.NoError(t, json.Unmarshal([]byte(body), &decoded)) + require.Len(t, decoded, 1) + require.NoError(t, validateDriftRecord(decoded[0])) + + empty, err := driftCorrectivePRBody([]driftRecord{}) + require.NoError(t, err) + var decodedEmpty []driftRecord + require.NoError(t, json.Unmarshal([]byte(empty), &decodedEmpty)) + assert.Empty(t, decodedEmpty) +} diff --git a/pkg/workflow/tools_validation_github_test.go b/pkg/workflow/tools_validation_github_test.go new file mode 100644 index 00000000000..6de21a7f930 --- /dev/null +++ b/pkg/workflow/tools_validation_github_test.go @@ -0,0 +1,38 @@ +//go:build !integration + +package workflow + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestEmitGitHubLockdownGuardPolicyWarningExactText validates that the +// compile-time lockdown/guard-policy conflict warning matches, byte-for-byte, +// the example message documented in +// scratchpad/github-mcp-access-control-specification.md §9.5.2. +func TestEmitGitHubLockdownGuardPolicyWarningExactText(t *testing.T) { + tools := NewTools(map[string]any{ + "github": map[string]any{ + "lockdown": true, + "allowed-repos": "all", + "min-integrity": "approved", + }, + }) + require.NoError(t, validateGitHubGuardPolicy(tools, "test-workflow")) + + compiler := NewCompiler() + stderrOutput := captureStderr(func() { + emitGitHubLockdownGuardPolicyWarning(compiler, tools, "test-workflow.md") + }) + + const specExampleMessage = `'tools.github.lockdown: true' is set; GitHub guard policy fields ('allowed-repos', 'min-integrity', 'blocked-users', 'trusted-users', 'approval-labels') will be ignored. +Guard policies are only evaluated when lockdown is not active.` + + assert.Equal(t, specExampleMessage, githubLockdownGuardPolicyWarningMessage, + "the implementation warning message must stay byte-identical to the §9.5.2 example") + assert.Contains(t, stderrOutput, specExampleMessage, + "the emitted warning must contain the exact §9.5.2 example message") +} diff --git a/scratchpad/github-mcp-access-control-specification.md b/scratchpad/github-mcp-access-control-specification.md index 50665922738..24425a65cff 100644 --- a/scratchpad/github-mcp-access-control-specification.md +++ b/scratchpad/github-mcp-access-control-specification.md @@ -1844,6 +1844,22 @@ Lockdown is an emergency or security stop that MUST NOT be weakened by other con See also: guard-policies-specification.md §Open Questions, decision record for question #3. +### 9.6 Safeguards + +This subsection specifies normative failure-mode behavior for the case where guard-policy configuration is malformed (for example, an unparseable `allowed-repos` value, an invalid `min-integrity` literal, or `blocked-users`/`trusted-users`/`approval-labels` present without a required `min-integrity`). + +- Implementations MUST reject a workflow at compile time when its guard-policy configuration is malformed; a malformed guard policy MUST NOT be silently ignored or silently coerced to a default value that widens access (see §4.7, `validateGitHubGuardPolicy()`). +- Implementations MUST fail closed on malformed guard-policy configuration: until the configuration error is fixed, the affected GitHub MCP tools MUST NOT be enabled, rather than falling back to an unrestricted ("all repos", `min-integrity: none`) policy. +- Compilation error messages for malformed guard-policy configuration SHOULD identify the offending field, the value received, and the set of valid values or formats, so the misconfiguration can be corrected without consulting this specification. +- Implementations MUST NOT partially apply a malformed guard policy (for example, enforcing `min-integrity` while ignoring an invalid `allowed-repos` value); validation MUST treat the guard policy as a single unit that either fully passes validation or causes compilation to fail. + +### 9.7 Open Questions + +1. **Should the `--strict` compile-time guard-policy dry-run report (§4.7, deferred design in guard-policies-specification.md Open Question #4) also surface the effective lockdown/guard-policy precedence outcome, so operators can see at a glance which fields are ignored?** + + **Decision**: Yes. The `--strict` dry-run report MUST include a `lockdown` indicator alongside the reported `allowed-repos`/`min-integrity`/`blocked-users`/`trusted-users`/`approval-labels` values, so that operators reviewing the report can immediately see that guard-policy fields are ignored at runtime when `lockdown: true` is set (§9.5.1). This is implemented in `pkg/cli/compile_guard_policy_report.go`. + *Rationale*: A dry-run report that omits the lockdown/guard-policy precedence relationship would be misleading — it would list "permitted" repositories that are, in fact, never consulted at runtime because lockdown supersedes them. Surfacing the precedence outcome directly in the report keeps the report consistent with the runtime enforcement behavior it is meant to preview, and reuses the same conflict-detection logic (`hasGitHubLockdownGuardPolicyConflict()`) already used for the compile-time warning (§9.5.2). + --- ## 10. Integration with MCP Gateway diff --git a/scratchpad/guard-policies-specification.md b/scratchpad/guard-policies-specification.md index 16c70d40aa6..0e7be8098dc 100644 --- a/scratchpad/guard-policies-specification.md +++ b/scratchpad/guard-policies-specification.md @@ -405,7 +405,7 @@ tools: 4. **Should we add a "dry-run" mode to test policies before enforcement?** - **Decision**: Dry-run enforcement mode is **deferred** to a future release. A compile-time validation (`gh aw compile --strict`) that reports which repositories would be permitted or denied under the configured guard policy SHOULD be implemented instead. + **Decision**: Runtime dry-run enforcement mode remains **deferred** to a future release. The compile-time validation (`gh aw compile --strict`) that reports which repositories would be permitted or denied under the configured guard policy is now **implemented**: `pkg/cli/compile_guard_policy_report.go` renders a per-workflow guard-policy dry-run report (allowed-repos, min-integrity, blocked-users, trusted-users, approval-labels, and lockdown precedence) to stderr whenever `--strict` is passed to `gh aw compile` and a GitHub guard policy is configured. *Rationale*: A runtime dry-run mode requires MCP Gateway support for pass-through logging of policy decisions, which is out of scope for the initial implementation. Compile-time policy analysis covers the majority of the validation need (catching misconfigured patterns before deployment) at lower implementation cost. Runtime dry-run may be added when MCP Gateway observability tooling matures. ## Conclusion diff --git a/scratchpad/safe-outputs-specification.md b/scratchpad/safe-outputs-specification.md index 594179a85bb..6bf025a4153 100644 --- a/scratchpad/safe-outputs-specification.md +++ b/scratchpad/safe-outputs-specification.md @@ -42,7 +42,9 @@ This specification is governed by the GitHub Next team and follows semantic vers 1. [Introduction](#1-introduction) 2. [Conformance](#2-conformance) + - [2.4 Norms](#24-norms) 3. [Architecture](#3-architecture) + - [3.6 Entities](#36-entities) 4. [Security Model](#4-security-model) 5. [Builtin System Tools](#5-builtin-system-tools) 6. [GitHub Operations](#6-github-operations) @@ -138,7 +140,7 @@ A **Complete Conforming Implementation** MUST satisfy Standard Conformance and: ### 2.2 Requirements Notation -The key words "MUST", "MUST NOT", "REQUIRED", "SHALL", "SHALL NOT", "SHOULD", "SHOULD NOT", "RECOMMENDED", "NOT RECOMMENDED", "MAY", and "OPTIONAL" in this document are to be interpreted as described in [RFC 2119](https://www.ietf.org/rfc/rfc2119.txt). +The key words "MUST", "MUST NOT", "REQUIRED", "SHALL", "SHALL NOT", "SHOULD", "SHOULD NOT", "RECOMMENDED", "NOT RECOMMENDED", "MAY", and "OPTIONAL" in this document are to be interpreted as described in [RFC 2119](https://www.ietf.org/rfc/rfc2119.txt). See §2.4 (Norms) for how these key words are applied consistently across this specification. ### 2.3 Compliance Levels @@ -148,6 +150,15 @@ Implementations are classified into three levels based on completeness: - **Level 2: Standard** - Common GitHub operations and guardrails - **Level 3: Complete** - Full feature set including advanced operations +### 2.4 Norms + +This subsection clarifies how the Requirements Notation (§2.2) is applied consistently throughout this specification, beyond the bare RFC 2119 definitions. + +- Every normative statement in this specification MUST use exactly one of the RFC 2119 key words; declarative sentences without a key word are informative only and MUST NOT be treated as conformance requirements. +- When a requirement uses "SHOULD" or "RECOMMENDED", implementations that deviate MUST document the deviation and its rationale (for example in release notes or an architecture decision record). +- Conflicting requirements MUST NOT appear for the same conformance class; if a later section appears to narrow an earlier "MUST", the later section is normative and the earlier section MUST be read as superseded. +- Requirements scoped to a specific conformance class (§2.1) apply only to implementations claiming that class or higher; a Standard Conforming Implementation MUST also satisfy all Basic Conformance requirements. + --- ## 3. Architecture @@ -371,6 +382,43 @@ Implementations MUST handle errors with: - **Descriptive Messages**: Clear error descriptions for debugging - **Graceful Degradation**: Continue processing remaining operations when possible +### 3.6 Entities + +This subsection formalizes the primary schema types referenced throughout this section and §6, defining their fields, types, and requirement level. + +#### 3.6.1 SafeOutputRequest + +The `SafeOutputRequest` entity represents a single validated NDJSON line written by the MCP server (§3.3.4) before it is consumed by an execution handler (§3.5). + +| Field | Type | Requirement | Description | +|-------|------|-------------|--------------| +| `type` | string | MUST | Normalized safe-output operation identifier (e.g. `create-issue`, `add-comment`) | +| `target` | string \| number | MAY | Resolution target per §3.4.4 (`"triggering"`, `"*"`, numeric issue/PR/discussion number, or temporary ID) | +| `target-repo` | string | MAY | Cross-repository target in `owner/repo` form, required only for cross-repository operations (§3.4.5) | +| `body` | string | SHOULD | Primary content payload for the operation; MUST be present for operations that render content | + +#### 3.6.2 GuardrailViolation + +The `GuardrailViolation` entity represents a rejected `SafeOutputRequest` that failed schema validation, max-count enforcement, sanitization, or target/cross-repository validation (§3.4). + +| Field | Type | Requirement | Description | +|-------|------|-------------|--------------| +| `code` | string | MUST | Error code from Appendix B (e.g. `E004`) | +| `type` | string | MUST | The safe-output operation type that was rejected | +| `reason` | string | MUST | Human-readable description of the validation failure | +| `field` | string | MAY | Name of the specific field that failed validation, when applicable | + +#### 3.6.3 ExecutionResult + +The `ExecutionResult` entity represents the outcome of an execution handler (§3.5) processing a single `SafeOutputRequest`. + +| Field | Type | Requirement | Description | +|-------|------|-------------|--------------| +| `type` | string | MUST | The safe-output operation type that was executed | +| `status` | string | MUST | One of `success`, `retried`, `failed`, `skipped` | +| `resource-url` | string | SHOULD | URL of the created/updated GitHub resource, when applicable | +| `error` | string | MAY | Error description, present only when `status` is `failed` | + --- ## 4. Security Model diff --git a/specs/aw-harness.md b/specs/aw-harness.md index 03936e93379..60ba5cc1915 100644 --- a/specs/aw-harness.md +++ b/specs/aw-harness.md @@ -1066,40 +1066,40 @@ This section specifies normative failure-mode responses that a conforming implem **Failure mode:** The Pi SDK package (`@earendil-works/pi-coding-agent`) or one of its core dependencies (`pi-agent-core`, `pi-ai`) cannot be loaded at harness startup (e.g., missing from bundle, corrupted installation, incompatible Node.js version). -**Normative response:** +**Normative response:** *(verified by [T-AW-006](#t-aw-006-pi-sdk-failure-to-load), §12)* -- The harness **MUST** catch the load error and emit a structured JSONL error event to stderr indicating the SDK load failure and the originating error message. -- The harness **MUST** write a human-readable error summary to `$GITHUB_STEP_SUMMARY` (if set) that identifies the failed module and suggests reinstalling or rebuilding the bundle. -- The harness **MUST** exit with code `2` (invocation error) rather than code `1` (session failure), to distinguish SDK infrastructure failures from session-level failures. -- The harness **MUST NOT** attempt to proceed with a partial or degraded session; no `AgentSession` **MUST** be created if the SDK cannot be loaded. +- The harness **MUST** catch the load error and emit a structured JSONL error event to stderr indicating the SDK load failure and the originating error message. *(T-AW-006)* +- The harness **MUST** write a human-readable error summary to `$GITHUB_STEP_SUMMARY` (if set) that identifies the failed module and suggests reinstalling or rebuilding the bundle. *(T-AW-006)* +- The harness **MUST** exit with code `2` (invocation error) rather than code `1` (session failure), to distinguish SDK infrastructure failures from session-level failures. *(T-AW-006)* +- The harness **MUST NOT** attempt to proceed with a partial or degraded session; no `AgentSession` **MUST** be created if the SDK cannot be loaded. *(T-AW-006)* #### 11.2.2 Budget Exhaustion **Failure mode:** The cumulative effective token count across all turns exceeds `harness.budget.max-effective-tokens`, or the cumulative AI credits consumed across all turns exceeds `harness.budget.max-ai-credits`, during an active session. -**Normative response:** +**Normative response:** *(verified by [T-AW-003](#t-aw-003-budget-gate), §12)* - When the budget metric reaches the **soft limit** (default: 80% of the configured limit), the cost-tracker extension **MUST** inject a steering message via `session.steer()` informing the agent that it is approaching the budget and **SHOULD** conclude its work soon. -- When the budget metric reaches the **hard limit**, the cost-tracker extension **MUST** abort the session immediately by invoking the session's abort API. The harness **MUST NOT** allow additional turns to proceed after the hard limit is reached. -- Upon hard-limit abort, the harness **MUST** emit a `budget_exceeded` JSONL event to stderr containing the final cumulative budget metric value and the configured limit. -- Upon hard-limit abort, the harness **MUST** append a `budget_exceeded` audit entry to the firewall audit log (`/tmp/gh-aw/sandbox/firewall/audit/log.jsonl`) so that the conclusion job can detect the condition without parsing stderr. The entry **MUST** include `"max_ai_credits_exceeded": true`, the final `"ai_credits"` consumed, and the configured `"max_ai_credits"` limit. When `max-effective-tokens` is the active budget key, the entry **MUST** still be written with `"max_ai_credits_exceeded": true` using an estimated AI-credits equivalent so that the conclusion job detection path is uniform across budget key types. +- When the budget metric reaches the **hard limit**, the cost-tracker extension **MUST** abort the session immediately by invoking the session's abort API. The harness **MUST NOT** allow additional turns to proceed after the hard limit is reached. *(T-AW-003)* +- Upon hard-limit abort, the harness **MUST** emit a `budget_exceeded` JSONL event to stderr containing the final cumulative budget metric value and the configured limit. *(T-AW-003)* +- Upon hard-limit abort, the harness **MUST** append a `budget_exceeded` audit entry to the firewall audit log (`/tmp/gh-aw/sandbox/firewall/audit/log.jsonl`) so that the conclusion job can detect the condition without parsing stderr. The entry **MUST** include `"max_ai_credits_exceeded": true`, the final `"ai_credits"` consumed, and the configured `"max_ai_credits"` limit. When `max-effective-tokens` is the active budget key, the entry **MUST** still be written with `"max_ai_credits_exceeded": true` using an estimated AI-credits equivalent so that the conclusion job detection path is uniform across budget key types. *(T-AW-003)* - The `budget_exceeded` event **MUST** explicitly signal forced termination (`reason: "hard_limit"` and `forced_termination: true`) so downstream consumers can distinguish budget aborts from other session failures. -- The harness **MUST** write a step summary entry to `$GITHUB_STEP_SUMMARY` (if set) indicating that the session was terminated due to budget exhaustion, showing the final metric value versus the limit. +- The harness **MUST** write a step summary entry to `$GITHUB_STEP_SUMMARY` (if set) indicating that the session was terminated due to budget exhaustion, showing the final metric value versus the limit. *(T-AW-003)* - On forced budget termination, the harness **MUST** preserve durable artifacts that were finalized before abort (`safe-outputs.ndjson` entries already appended, JSONL events already emitted, and step-summary rows for completed turns). - On forced budget termination, the harness **MUST** discard in-flight turn state that did not reach a completed turn boundary (partial assistant output, partially collected tool results, and uncommitted per-turn aggregates). - A "completed turn boundary" means the `turn_end` event has been emitted and all per-turn persistence for that turn (JSONL line, counters, and step-summary row) has succeeded. -- The harness **MUST** exit with code `1` (session failure) after a hard-limit abort, so that the GitHub Actions job is marked as failed. +- The harness **MUST** exit with code `1` (session failure) after a hard-limit abort, so that the GitHub Actions job is marked as failed. *(T-AW-003)* #### 11.2.3 Extension Crash Isolation **Failure mode:** A user-supplied Pi extension (declared via `harness.extensions`) throws an uncaught exception or returns a rejected Promise during its initialization function, or throws during event handler execution. -**Normative response:** +**Normative response:** *(verified by [T-AW-002](#t-aw-002-extension-loading) and [T-AW-007](#t-aw-007-extension-crash-isolation), §12)* -- **During initialization:** If an extension's default export function throws or rejects, the harness **MUST** catch the error, emit a warning to stderr identifying the failing extension by name/path and the error message, and continue loading the remaining extensions. The failing extension **MUST** be skipped and **MUST NOT** be registered into the session. If `harness.extensions-required: true` is set, the harness **MUST** instead abort startup with exit code `2` and a descriptive error message. +- **During initialization:** If an extension's default export function throws or rejects, the harness **MUST** catch the error, emit a warning to stderr identifying the failing extension by name/path and the error message, and continue loading the remaining extensions. The failing extension **MUST** be skipped and **MUST NOT** be registered into the session. If `harness.extensions-required: true` is set, the harness **MUST** instead abort startup with exit code `2` and a descriptive error message. *(T-AW-002, T-AW-007)* - **During event handling:** If an extension's event handler (registered via `pi.on()`) throws or rejects, the Pi SDK event dispatch **MUST** catch the error. If the Pi SDK does not isolate handler errors, the harness **MUST** wrap all user extension event handlers in a try/catch that emits a structured JSONL warning and allows the session to continue. - **Built-in extensions are never skipped:** The five built-in gh-aw extensions (provider setup, cost-tracker, steering, repair, observability) **MUST NOT** be subject to the skip-on-error policy described above. If a built-in extension fails to load, the harness **MUST** treat it as a fatal startup error and exit with code `2`. -- The harness **MUST NOT** allow a crashing user extension to terminate the entire harness process without first completing the cleanup described above (step summary, final JSONL event). +- The harness **MUST NOT** allow a crashing user extension to terminate the entire harness process without first completing the cleanup described above (step summary, final JSONL event). *(T-AW-007)* ### 11.3 MUST/MUST NOT Traceability (Spec ↔ Harness Source) diff --git a/specs/awf-config-sources-compliance/README.md b/specs/awf-config-sources-compliance/README.md index b5d2634dbdd..ccb7fb1189f 100644 --- a/specs/awf-config-sources-compliance/README.md +++ b/specs/awf-config-sources-compliance/README.md @@ -37,13 +37,13 @@ The following test IDs cover the `DriftRecord` schema and its usage requirements ## Running Conformance Tests -Conformance tests that validate `DriftRecord` schema compliance will be located in (or added to): +Conformance tests that validate `DriftRecord` schema compliance are implemented in: ``` -pkg/workflow/awf_config_drift_test.go — DriftRecord schema validation (T-DR-001 through T-DR-005) +pkg/workflow/awf_config_drift_test.go — DriftRecord schema validation and usage (T-DR-001 through T-DR-010) ``` -To run related tests (once implemented): +To run related tests: ```bash go test -v -run "TestDriftRecord" ./pkg/workflow/