Skip to content

Commit 3a94637

Browse files
authored
fix: improve DIFC error messages and replace issue:#0 sentinel (#2202)
## Summary Improves DIFC error messaging for non-expert users and fixes the synthetic `issue:#0` resource descriptions reported in [gh-aw#21866 (recommendation 5)](github/gh-aw#21866). Addresses [gh-aw#21824](github/gh-aw#21824) — confusing "Agent would need to drop integrity tags" messages. ## Changes ### Guard (Rust) — Replace `#0` sentinel with `#unknown` - New `extract_resource_number()` helper in `helpers.rs` returns `"unknown"` (with `log_warn`) when the `number` field is missing or invalid - Replaces `unwrap_or(0)` in both `response_items.rs` and `response_paths.rs` for issues and PRs - **Before**: `issue:github/gh-aw#0` → **After**: `issue:github/gh-aw#unknown` ### DIFC Evaluator (Go) — Human-readable error messages | Context | Before | After | |---------|--------|-------| | Read denied (integrity) | "Agent would need to drop integrity tags [unapproved:all approved:all] to trust this resource" | "The agent cannot read data with integrity below \"approved\"" | | Read denied (secrecy) | "Agent would need to add secrecy tags [private:org/repo] to read this resource" | "The agent is not authorized to access private (org/repo)-scoped data" | | Write denied (integrity) | "Resource requires integrity tags [production] that agent doesn't have" | "The agent's integrity level is insufficient; it needs \"production\" integrity" | | Write denied (secrecy) | "Resource would need these secrecy requirements to accept the write" | "The agent carries sensitive data that the target resource is not authorized to receive" | ### New helper functions - `formatIntegrityLevel()` — converts tag lists to named levels (e.g., `[unapproved:all approved:all]` → `"approved"`) - `formatSecrecyLevel()` — converts tags to scope descriptions (e.g., `[private:org/repo]` → `"private (org/repo)"`) ## Test Results - All 70 Rust guard tests pass ✅ - All Go unit tests pass (including updated assertion expectations) ✅ - 0 lint issues ✅
2 parents 2e4bc62 + 1c8ba56 commit 3a94637

6 files changed

Lines changed: 102 additions & 30 deletions

File tree

guards/github-guard/rust-guard/src/labels/helpers.rs

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,21 @@ use serde_json::Value;
77

88
use super::constants::{field_names, label_constants};
99

10+
/// Extract a resource number from a JSON item, returning the number as a string.
11+
/// Returns "unknown" (with a log warning) if the number field is missing or invalid.
12+
pub(crate) fn extract_resource_number(item: &Value, resource_type: &str, repo: &str) -> String {
13+
match item.get("number").and_then(|v| v.as_u64()) {
14+
Some(n) => n.to_string(),
15+
None => {
16+
crate::log_warn(&format!(
17+
"{}:{} — missing or invalid 'number' field, using 'unknown'",
18+
resource_type, repo
19+
));
20+
"unknown".to_string()
21+
}
22+
}
23+
}
24+
1025
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
1126
pub enum ScopeKind {
1227
All,

guards/github-guard/rust-guard/src/labels/response_items.rs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -113,7 +113,7 @@ pub fn label_response_items(
113113
};
114114

115115
for item in items_to_process.iter() {
116-
let number = item.get("number").and_then(|v| v.as_i64()).unwrap_or(0);
116+
let number = extract_resource_number(item, "pr", &arg_repo_full);
117117

118118
// Get repo info from the PR's base or head
119119
let repo_full_name = item
@@ -205,7 +205,7 @@ pub fn label_response_items(
205205

206206
let repo_private = repo_visibility_private_for_repo_id(&repo_full_name)
207207
.unwrap_or(default_repo_private);
208-
let number = item.get("number").and_then(|v| v.as_i64()).unwrap_or(0);
208+
let number = extract_resource_number(item, "issue", &repo_full_name);
209209
let integrity = issue_integrity(
210210
item,
211211
&repo_full_name,

guards/github-guard/rust-guard/src/labels/response_paths.rs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -145,7 +145,7 @@ pub fn label_response_paths(
145145
let item_repo_private = repo_visibility_private_for_repo_id(repo_for_labels)
146146
.unwrap_or(default_repo_private);
147147

148-
let pr_number = item.get("number").and_then(|v| v.as_u64()).unwrap_or(0);
148+
let pr_number = extract_resource_number(item, "pr", repo_for_labels);
149149
let integrity =
150150
pr_integrity(item, repo_for_labels, item_repo_private, is_forked, ctx);
151151
let path = make_item_path(items_path, i);
@@ -224,7 +224,7 @@ pub fn label_response_paths(
224224
let item_repo_private = repo_visibility_private_for_repo_id(repo_for_labels)
225225
.unwrap_or(default_repo_private);
226226

227-
let issue_number = item.get("number").and_then(|v| v.as_u64()).unwrap_or(0);
227+
let issue_number = extract_resource_number(item, "issue", repo_for_labels);
228228
let integrity = issue_integrity(
229229
item,
230230
repo_for_labels,

internal/difc/evaluator.go

Lines changed: 73 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -155,6 +155,70 @@ func (e *Evaluator) GetMode() EnforcementMode {
155155
return e.mode
156156
}
157157

158+
// formatIntegrityLevel converts a list of integrity tags into a human-readable
159+
// integrity level description (e.g., "approved" instead of "[unapproved:all approved:all]").
160+
func formatIntegrityLevel(tags []Tag) string {
161+
if len(tags) == 0 {
162+
return "none"
163+
}
164+
// Find the highest integrity level mentioned in the tags
165+
highest := ""
166+
for _, tag := range tags {
167+
s := string(tag)
168+
// Strip scope suffix (e.g., "approved:all" → "approved")
169+
if idx := strings.Index(s, ":"); idx > 0 {
170+
s = s[:idx]
171+
}
172+
switch s {
173+
case "merged":
174+
return "\"merged\""
175+
case "approved":
176+
highest = "\"approved\""
177+
case "unapproved":
178+
if highest == "" {
179+
highest = "\"unapproved\""
180+
}
181+
}
182+
}
183+
if highest != "" {
184+
return highest
185+
}
186+
return fmt.Sprintf("%v", tags)
187+
}
188+
189+
// formatSecrecyLevel converts a list of secrecy tags into a human-readable
190+
// secrecy scope description (e.g., "private (org/repo)" instead of "[private:org/repo]").
191+
func formatSecrecyLevel(tags []Tag) string {
192+
if len(tags) == 0 {
193+
return "public"
194+
}
195+
196+
bestScope := ""
197+
hasPrivate := false
198+
199+
for _, tag := range tags {
200+
s := string(tag)
201+
if strings.HasPrefix(s, "private:") {
202+
scope := strings.TrimPrefix(s, "private:")
203+
if scope != "" && len(scope) > len(bestScope) {
204+
bestScope = scope
205+
}
206+
continue
207+
}
208+
if s == "private" {
209+
hasPrivate = true
210+
}
211+
}
212+
213+
if bestScope != "" {
214+
return fmt.Sprintf("private (%s)", bestScope)
215+
}
216+
if hasPrivate {
217+
return "private"
218+
}
219+
return fmt.Sprintf("%v", tags)
220+
}
221+
158222
// newEmptyEvaluationResult creates a new EvaluationResult with default initialization.
159223
// This helper centralizes the common pattern of creating an empty result with AccessAllow decision
160224
// and empty tag slices, reducing duplication across evaluation functions.
@@ -249,8 +313,8 @@ func (e *Evaluator) evaluateRead(
249313
result.Decision = AccessDeny
250314
result.IntegrityToDrop = integrityMissingTags
251315
result.Reason = fmt.Sprintf("Resource '%s' has lower integrity than agent requires. "+
252-
"Agent would need to drop integrity tags %v to trust this resource.",
253-
resource.Description, integrityMissingTags)
316+
"The agent cannot read data with integrity below %s.",
317+
resource.Description, formatIntegrityLevel(integrityMissingTags))
254318
return result
255319
}
256320

@@ -259,8 +323,8 @@ func (e *Evaluator) evaluateRead(
259323
result.Decision = AccessDeny
260324
result.SecrecyToAdd = secrecyExtraTags
261325
result.Reason = fmt.Sprintf("Resource '%s' has secrecy requirements that agent doesn't meet. "+
262-
"Agent would need to add secrecy tags %v to read this resource.",
263-
resource.Description, secrecyExtraTags)
326+
"The agent is not authorized to access %s-scoped data.",
327+
resource.Description, formatSecrecyLevel(secrecyExtraTags))
264328
return result
265329
}
266330

@@ -287,8 +351,8 @@ func (e *Evaluator) evaluateWrite(
287351
result.Decision = AccessDeny
288352
result.IntegrityToDrop = missingTags
289353
result.Reason = fmt.Sprintf("Agent lacks required integrity to write to '%s'. "+
290-
"Resource requires integrity tags %v that agent doesn't have.",
291-
resource.Description, missingTags)
354+
"The agent's integrity level is insufficient; it needs %s integrity.",
355+
resource.Description, formatIntegrityLevel(missingTags))
292356
return result
293357
}
294358

@@ -300,9 +364,9 @@ func (e *Evaluator) evaluateWrite(
300364
logEvaluator.Printf("Write denied: secrecy check failed, extraTags=%v", extraTags)
301365
result.Decision = AccessDeny
302366
result.SecrecyToAdd = extraTags
303-
result.Reason = fmt.Sprintf("Agent has secrecy tags %v that cannot flow to '%s'. "+
304-
"Resource would need these secrecy requirements to accept the write.",
305-
extraTags, resource.Description)
367+
result.Reason = fmt.Sprintf("Agent carries %s-scoped data that cannot be written to '%s' due to secrecy constraints. "+
368+
"The target resource is not authorized to receive this sensitive data.",
369+
formatSecrecyLevel(extraTags), resource.Description)
306370
return result
307371
}
308372

internal/difc/labels.go

Lines changed: 3 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -373,21 +373,18 @@ func (e *ViolationError) Error() string {
373373
if e.Type == SecrecyViolation {
374374
msg = fmt.Sprintf("Secrecy violation for resource '%s': ", e.Resource)
375375
if len(e.ExtraTags) > 0 {
376-
msg += fmt.Sprintf("agent has secrecy tags %v that cannot flow to resource. ", e.ExtraTags)
377-
msg += "Remediation: remove these tags from agent's secrecy label or add them to the resource's secrecy requirements."
376+
msg += fmt.Sprintf("the agent is not authorized to access data with secrecy level %s.", formatSecrecyLevel(e.ExtraTags))
378377
}
379378
} else {
380379
if e.IsWrite {
381380
msg = fmt.Sprintf("Integrity violation for write to resource '%s': ", e.Resource)
382381
if len(e.MissingTags) > 0 {
383-
msg += fmt.Sprintf("agent is missing required integrity tags %v. ", e.MissingTags)
384-
msg += fmt.Sprintf("Remediation: agent must gain integrity tags %v to write to this resource.", e.MissingTags)
382+
msg += fmt.Sprintf("the agent's integrity level is insufficient; it needs %s integrity.", formatIntegrityLevel(e.MissingTags))
385383
}
386384
} else {
387385
msg = fmt.Sprintf("Integrity violation for read from resource '%s': ", e.Resource)
388386
if len(e.MissingTags) > 0 {
389-
msg += fmt.Sprintf("resource is missing integrity tags %v that agent requires. ", e.MissingTags)
390-
msg += fmt.Sprintf("Remediation: agent should drop integrity tags %v to trust this resource, or verify resource has higher integrity.", e.MissingTags)
387+
msg += fmt.Sprintf("the agent cannot read data with integrity below %s.", formatIntegrityLevel(e.MissingTags))
391388
}
392389
}
393390
}

internal/difc/labels_test.go

Lines changed: 7 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -507,9 +507,7 @@ func TestViolationError_Error(t *testing.T) {
507507
wantContains: []string{
508508
"Secrecy violation",
509509
"classified-doc",
510-
"[secret top-secret]",
511-
"cannot flow to resource",
512-
"Remediation:",
510+
"not authorized to access",
513511
},
514512
},
515513
{
@@ -523,8 +521,8 @@ func TestViolationError_Error(t *testing.T) {
523521
"Secrecy violation",
524522
"public-endpoint",
525523
},
526-
// No tag list or remediation when ExtraTags is empty
527-
wantAbsent: []string{"cannot flow to resource"},
524+
// No tag list when ExtraTags is empty
525+
wantAbsent: []string{"not authorized"},
528526
},
529527
{
530528
name: "integrity write violation with missing tags",
@@ -537,8 +535,7 @@ func TestViolationError_Error(t *testing.T) {
537535
wantContains: []string{
538536
"Integrity violation for write",
539537
"prod-db",
540-
"missing required integrity tags",
541-
"Remediation:",
538+
"integrity level is insufficient",
542539
"production",
543540
"verified",
544541
},
@@ -555,7 +552,7 @@ func TestViolationError_Error(t *testing.T) {
555552
"Integrity violation for write",
556553
"prod-db",
557554
},
558-
wantAbsent: []string{"missing required integrity tags"},
555+
wantAbsent: []string{"integrity level is insufficient"},
559556
},
560557
{
561558
name: "integrity read violation with missing tags",
@@ -568,8 +565,7 @@ func TestViolationError_Error(t *testing.T) {
568565
wantContains: []string{
569566
"Integrity violation for read",
570567
"trusted-source",
571-
"missing integrity tags",
572-
"Remediation:",
568+
"cannot read data with integrity below",
573569
"certified",
574570
},
575571
},
@@ -585,7 +581,7 @@ func TestViolationError_Error(t *testing.T) {
585581
"Integrity violation for read",
586582
"trusted-source",
587583
},
588-
wantAbsent: []string{"missing integrity tags"},
584+
wantAbsent: []string{"cannot read data"},
589585
},
590586
}
591587

0 commit comments

Comments
 (0)