From e29f698795a6e5f9833f1e6ec2cad03f94942f9d Mon Sep 17 00:00:00 2001 From: Landon Cox Date: Thu, 19 Mar 2026 20:17:59 -0700 Subject: [PATCH 1/6] fix: improve DIFC error messages and replace issue:#0 sentinel MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Guard improvements (Rust): - Replace unwrap_or(0) with extract_resource_number() helper that returns "unknown" instead of "0" for missing/invalid number fields - Log warnings when number field is missing (helps diagnose synthetic issue:#0 entries reported in DIFC analysis discussions) - Applies to both issue and PR number extraction in response_items.rs and response_paths.rs Error message improvements (Go): - Replace technical "Agent would need to drop integrity tags [X]" with human-readable "The agent cannot read data with integrity below X" - Replace "Agent would need to add secrecy tags" with "The agent is not authorized to access private-scoped data" - Add formatIntegrityLevel() that converts tag lists to named levels (e.g., [unapproved:all approved:all] → "approved") - Add formatSecrecyLevel() that converts tags to scope descriptions (e.g., [private:org/repo] → "private (org/repo)") - Update ViolationError.Error() with consistent plain-language messages - Remove verbose "Remediation:" hints from user-facing errors Addresses: github/gh-aw#21824, github/gh-aw#21866 (recommendation 5) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../rust-guard/src/labels/helpers.rs | 15 +++++ .../rust-guard/src/labels/response_items.rs | 4 +- .../rust-guard/src/labels/response_paths.rs | 4 +- internal/difc/evaluator.go | 67 +++++++++++++++++-- internal/difc/labels.go | 9 +-- internal/difc/labels_test.go | 18 ++--- 6 files changed, 89 insertions(+), 28 deletions(-) diff --git a/guards/github-guard/rust-guard/src/labels/helpers.rs b/guards/github-guard/rust-guard/src/labels/helpers.rs index 7a924a60..45fcf892 100644 --- a/guards/github-guard/rust-guard/src/labels/helpers.rs +++ b/guards/github-guard/rust-guard/src/labels/helpers.rs @@ -7,6 +7,21 @@ use serde_json::Value; use super::constants::{field_names, label_constants}; +/// Extract a resource number from a JSON item, returning the number as a string. +/// Returns "unknown" (with a log warning) if the number field is missing or invalid. +pub(crate) fn extract_resource_number(item: &Value, resource_type: &str, repo: &str) -> String { + match item.get("number").and_then(|v| v.as_u64()) { + Some(n) => n.to_string(), + None => { + crate::log_warn(&format!( + "{}:{} — missing or invalid 'number' field, using 'unknown'", + resource_type, repo + )); + "unknown".to_string() + } + } +} + #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub enum ScopeKind { All, diff --git a/guards/github-guard/rust-guard/src/labels/response_items.rs b/guards/github-guard/rust-guard/src/labels/response_items.rs index 6676bef2..24cea931 100644 --- a/guards/github-guard/rust-guard/src/labels/response_items.rs +++ b/guards/github-guard/rust-guard/src/labels/response_items.rs @@ -113,7 +113,7 @@ pub fn label_response_items( }; for item in items_to_process.iter() { - let number = item.get("number").and_then(|v| v.as_i64()).unwrap_or(0); + let number = extract_resource_number(item, "pr", &arg_repo_full); // Get repo info from the PR's base or head let repo_full_name = item @@ -205,7 +205,7 @@ pub fn label_response_items( let repo_private = repo_visibility_private_for_repo_id(&repo_full_name) .unwrap_or(default_repo_private); - let number = item.get("number").and_then(|v| v.as_i64()).unwrap_or(0); + let number = extract_resource_number(item, "issue", &repo_full_name); let integrity = issue_integrity( item, &repo_full_name, diff --git a/guards/github-guard/rust-guard/src/labels/response_paths.rs b/guards/github-guard/rust-guard/src/labels/response_paths.rs index ed927b32..b65b7df2 100644 --- a/guards/github-guard/rust-guard/src/labels/response_paths.rs +++ b/guards/github-guard/rust-guard/src/labels/response_paths.rs @@ -145,7 +145,7 @@ pub fn label_response_paths( let item_repo_private = repo_visibility_private_for_repo_id(repo_for_labels) .unwrap_or(default_repo_private); - let pr_number = item.get("number").and_then(|v| v.as_u64()).unwrap_or(0); + let pr_number = extract_resource_number(item, "pr", repo_for_labels); let integrity = pr_integrity(item, repo_for_labels, item_repo_private, is_forked, ctx); let path = make_item_path(items_path, i); @@ -224,7 +224,7 @@ pub fn label_response_paths( let item_repo_private = repo_visibility_private_for_repo_id(repo_for_labels) .unwrap_or(default_repo_private); - let issue_number = item.get("number").and_then(|v| v.as_u64()).unwrap_or(0); + let issue_number = extract_resource_number(item, "issue", repo_for_labels); let integrity = issue_integrity( item, repo_for_labels, diff --git a/internal/difc/evaluator.go b/internal/difc/evaluator.go index 4fbd5329..70040684 100644 --- a/internal/difc/evaluator.go +++ b/internal/difc/evaluator.go @@ -155,6 +155,59 @@ func (e *Evaluator) GetMode() EnforcementMode { return e.mode } +// formatIntegrityLevel converts a list of integrity tags into a human-readable +// integrity level description (e.g., "approved" instead of "[unapproved:all approved:all]"). +func formatIntegrityLevel(tags []Tag) string { + if len(tags) == 0 { + return "none" + } + // Find the highest integrity level mentioned in the tags + highest := "" + for _, tag := range tags { + s := string(tag) + // Strip scope suffix (e.g., "approved:all" → "approved") + if idx := strings.Index(s, ":"); idx > 0 { + s = s[:idx] + } + switch s { + case "merged": + return "\"merged\"" + case "approved": + highest = "\"approved\"" + case "unapproved": + if highest == "" { + highest = "\"unapproved\"" + } + } + } + if highest != "" { + return highest + } + return fmt.Sprintf("%v", tags) +} + +// formatSecrecyLevel converts a list of secrecy tags into a human-readable +// secrecy scope description (e.g., "private" instead of "[private:org/repo]"). +func formatSecrecyLevel(tags []Tag) string { + if len(tags) == 0 { + return "public" + } + for _, tag := range tags { + s := string(tag) + if strings.HasPrefix(s, "private:") { + scope := strings.TrimPrefix(s, "private:") + if scope != "" { + return fmt.Sprintf("private (%s)", scope) + } + return "private" + } + if s == "private" { + return "private" + } + } + return fmt.Sprintf("%v", tags) +} + // newEmptyEvaluationResult creates a new EvaluationResult with default initialization. // This helper centralizes the common pattern of creating an empty result with AccessAllow decision // and empty tag slices, reducing duplication across evaluation functions. @@ -249,8 +302,8 @@ func (e *Evaluator) evaluateRead( result.Decision = AccessDeny result.IntegrityToDrop = integrityMissingTags result.Reason = fmt.Sprintf("Resource '%s' has lower integrity than agent requires. "+ - "Agent would need to drop integrity tags %v to trust this resource.", - resource.Description, integrityMissingTags) + "The agent cannot read data with integrity below %s.", + resource.Description, formatIntegrityLevel(integrityMissingTags)) return result } @@ -259,8 +312,8 @@ func (e *Evaluator) evaluateRead( result.Decision = AccessDeny result.SecrecyToAdd = secrecyExtraTags result.Reason = fmt.Sprintf("Resource '%s' has secrecy requirements that agent doesn't meet. "+ - "Agent would need to add secrecy tags %v to read this resource.", - resource.Description, secrecyExtraTags) + "The agent is not authorized to access %s-scoped data.", + resource.Description, formatSecrecyLevel(secrecyExtraTags)) return result } @@ -287,8 +340,8 @@ func (e *Evaluator) evaluateWrite( result.Decision = AccessDeny result.IntegrityToDrop = missingTags result.Reason = fmt.Sprintf("Agent lacks required integrity to write to '%s'. "+ - "Resource requires integrity tags %v that agent doesn't have.", - resource.Description, missingTags) + "The agent's integrity level is insufficient; it needs %s integrity.", + resource.Description, formatIntegrityLevel(missingTags)) return result } @@ -301,7 +354,7 @@ func (e *Evaluator) evaluateWrite( result.Decision = AccessDeny result.SecrecyToAdd = extraTags result.Reason = fmt.Sprintf("Agent has secrecy tags %v that cannot flow to '%s'. "+ - "Resource would need these secrecy requirements to accept the write.", + "The agent carries sensitive data that the target resource is not authorized to receive.", extraTags, resource.Description) return result } diff --git a/internal/difc/labels.go b/internal/difc/labels.go index 2a11ffdd..f90953a1 100644 --- a/internal/difc/labels.go +++ b/internal/difc/labels.go @@ -373,21 +373,18 @@ func (e *ViolationError) Error() string { if e.Type == SecrecyViolation { msg = fmt.Sprintf("Secrecy violation for resource '%s': ", e.Resource) if len(e.ExtraTags) > 0 { - msg += fmt.Sprintf("agent has secrecy tags %v that cannot flow to resource. ", e.ExtraTags) - msg += "Remediation: remove these tags from agent's secrecy label or add them to the resource's secrecy requirements." + msg += fmt.Sprintf("the agent is not authorized to access %s-scoped data.", formatSecrecyLevel(e.ExtraTags)) } } else { if e.IsWrite { msg = fmt.Sprintf("Integrity violation for write to resource '%s': ", e.Resource) if len(e.MissingTags) > 0 { - msg += fmt.Sprintf("agent is missing required integrity tags %v. ", e.MissingTags) - msg += fmt.Sprintf("Remediation: agent must gain integrity tags %v to write to this resource.", e.MissingTags) + msg += fmt.Sprintf("the agent's integrity level is insufficient; it needs %s integrity.", formatIntegrityLevel(e.MissingTags)) } } else { msg = fmt.Sprintf("Integrity violation for read from resource '%s': ", e.Resource) if len(e.MissingTags) > 0 { - msg += fmt.Sprintf("resource is missing integrity tags %v that agent requires. ", e.MissingTags) - msg += fmt.Sprintf("Remediation: agent should drop integrity tags %v to trust this resource, or verify resource has higher integrity.", e.MissingTags) + msg += fmt.Sprintf("the agent cannot read data with integrity below %s.", formatIntegrityLevel(e.MissingTags)) } } } diff --git a/internal/difc/labels_test.go b/internal/difc/labels_test.go index f9fd2ed4..d3968d4b 100644 --- a/internal/difc/labels_test.go +++ b/internal/difc/labels_test.go @@ -507,9 +507,7 @@ func TestViolationError_Error(t *testing.T) { wantContains: []string{ "Secrecy violation", "classified-doc", - "[secret top-secret]", - "cannot flow to resource", - "Remediation:", + "not authorized to access", }, }, { @@ -523,8 +521,8 @@ func TestViolationError_Error(t *testing.T) { "Secrecy violation", "public-endpoint", }, - // No tag list or remediation when ExtraTags is empty - wantAbsent: []string{"cannot flow to resource"}, + // No tag list when ExtraTags is empty + wantAbsent: []string{"not authorized"}, }, { name: "integrity write violation with missing tags", @@ -537,8 +535,7 @@ func TestViolationError_Error(t *testing.T) { wantContains: []string{ "Integrity violation for write", "prod-db", - "missing required integrity tags", - "Remediation:", + "integrity level is insufficient", "production", "verified", }, @@ -555,7 +552,7 @@ func TestViolationError_Error(t *testing.T) { "Integrity violation for write", "prod-db", }, - wantAbsent: []string{"missing required integrity tags"}, + wantAbsent: []string{"integrity level is insufficient"}, }, { name: "integrity read violation with missing tags", @@ -568,8 +565,7 @@ func TestViolationError_Error(t *testing.T) { wantContains: []string{ "Integrity violation for read", "trusted-source", - "missing integrity tags", - "Remediation:", + "cannot read data with integrity below", "certified", }, }, @@ -585,7 +581,7 @@ func TestViolationError_Error(t *testing.T) { "Integrity violation for read", "trusted-source", }, - wantAbsent: []string{"missing integrity tags"}, + wantAbsent: []string{"cannot read data"}, }, } From f312d0fe1955f39c78a0247c4badd38d4d18981f Mon Sep 17 00:00:00 2001 From: Landon Cox Date: Thu, 19 Mar 2026 21:18:08 -0700 Subject: [PATCH 2/6] Update internal/difc/evaluator.go Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- internal/difc/evaluator.go | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-) diff --git a/internal/difc/evaluator.go b/internal/difc/evaluator.go index 70040684..b119a35b 100644 --- a/internal/difc/evaluator.go +++ b/internal/difc/evaluator.go @@ -192,19 +192,30 @@ func formatSecrecyLevel(tags []Tag) string { if len(tags) == 0 { return "public" } + + bestScope := "" + hasPrivate := false + for _, tag := range tags { s := string(tag) if strings.HasPrefix(s, "private:") { scope := strings.TrimPrefix(s, "private:") - if scope != "" { - return fmt.Sprintf("private (%s)", scope) + if scope != "" && len(scope) > len(bestScope) { + bestScope = scope } - return "private" + continue } if s == "private" { - return "private" + hasPrivate = true } } + + if bestScope != "" { + return fmt.Sprintf("private (%s)", bestScope) + } + if hasPrivate { + return "private" + } return fmt.Sprintf("%v", tags) } From 4bcb737cb72302de49de0608dcd5d04c1c5bedb9 Mon Sep 17 00:00:00 2001 From: Landon Cox Date: Thu, 19 Mar 2026 21:18:32 -0700 Subject: [PATCH 3/6] Update internal/difc/evaluator.go Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- internal/difc/evaluator.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/difc/evaluator.go b/internal/difc/evaluator.go index b119a35b..294c3355 100644 --- a/internal/difc/evaluator.go +++ b/internal/difc/evaluator.go @@ -187,7 +187,7 @@ func formatIntegrityLevel(tags []Tag) string { } // formatSecrecyLevel converts a list of secrecy tags into a human-readable -// secrecy scope description (e.g., "private" instead of "[private:org/repo]"). +// secrecy scope description (e.g., "private (org/repo)" instead of "[private:org/repo]"). func formatSecrecyLevel(tags []Tag) string { if len(tags) == 0 { return "public" From bcefb6fcc698bb1f8636770da19830a60c3c04a2 Mon Sep 17 00:00:00 2001 From: Landon Cox Date: Thu, 19 Mar 2026 21:19:11 -0700 Subject: [PATCH 4/6] Update internal/difc/labels.go Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- internal/difc/labels.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/difc/labels.go b/internal/difc/labels.go index f90953a1..8d5ac740 100644 --- a/internal/difc/labels.go +++ b/internal/difc/labels.go @@ -373,7 +373,7 @@ func (e *ViolationError) Error() string { if e.Type == SecrecyViolation { msg = fmt.Sprintf("Secrecy violation for resource '%s': ", e.Resource) if len(e.ExtraTags) > 0 { - msg += fmt.Sprintf("the agent is not authorized to access %s-scoped data.", formatSecrecyLevel(e.ExtraTags)) + msg += fmt.Sprintf("the agent is not authorized to access data with secrecy level %s.", formatSecrecyLevel(e.ExtraTags)) } } else { if e.IsWrite { From 00bfa2e8477a850d65c764eb108fed6623f00ff1 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 20 Mar 2026 04:19:28 +0000 Subject: [PATCH 5/6] Initial plan From 5fab0838609486c23a12d9ab6dfc1876eab7b409 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 20 Mar 2026 04:25:23 +0000 Subject: [PATCH 6/6] fix: drop raw tag list from write-secrecy denial Reason message Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com> Agent-Logs-Url: https://github.com/github/gh-aw-mcpg/sessions/cfe43072-48d3-4ed2-bad2-4583d326138a --- internal/difc/evaluator.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/internal/difc/evaluator.go b/internal/difc/evaluator.go index 294c3355..e7ba75d6 100644 --- a/internal/difc/evaluator.go +++ b/internal/difc/evaluator.go @@ -364,9 +364,9 @@ func (e *Evaluator) evaluateWrite( logEvaluator.Printf("Write denied: secrecy check failed, extraTags=%v", extraTags) result.Decision = AccessDeny result.SecrecyToAdd = extraTags - result.Reason = fmt.Sprintf("Agent has secrecy tags %v that cannot flow to '%s'. "+ - "The agent carries sensitive data that the target resource is not authorized to receive.", - extraTags, resource.Description) + result.Reason = fmt.Sprintf("Agent carries %s-scoped data that cannot be written to '%s' due to secrecy constraints. "+ + "The target resource is not authorized to receive this sensitive data.", + formatSecrecyLevel(extraTags), resource.Description) return result }