SPDD 2026-07-05: outcome evaluators, policy compiler, and spec coverage for batch 20-24#43594
Conversation
…e fixtures Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Hey
If you'd like to split this into more focused units, here's a ready-to-use prompt:
|
There was a problem hiding this comment.
Pull request overview
This PR advances SPDD batch 20–24 coverage by adding missing Go outcome evaluators, introducing a first-pass intent execution policy compiler, and extending several specs/fixture sets (safe-output outcomes, replace-label compliance, GitHub MCP access-control compliance, and OTel observability test stubs).
Changes:
- Added Go outcome evaluators for
dispatch_workflow(REST run status mapping) andupdate_discussion(pending GraphQL), and registered them in the evaluator map. - Introduced
ExecutionPolicy/PolicyCompilerwith “more-restrictive-wins” merging, plus initial unit tests. - Expanded specs and added new compliance fixtures for replace-label and GitHub MCP access control; added OTel Level 1 test ID stubs.
Show a summary per file
| File | Description |
|---|---|
| specs/safe-output-outcome-evaluation.md | Updates evaluator mapping/implementation status tables for dispatch_workflow, update_discussion, and replace_label. |
| specs/replace-label-compliance/rl-003-blocklist-ordering.yaml | New replace-label compliance fixture covering blocklist-before-allowlist ordering. |
| specs/replace-label-compliance/rl-001-glob-semantics.yaml | New replace-label compliance fixture covering glob semantics expectations. |
| specs/replace-label-compliance/README.md | Documents fixture intent and schema for replace-label compliance fixtures. |
| specs/otel-observability-spec.md | Adds Level 1 OTel compliance test ID stubs (T-OT-001–T-OT-007). |
| specs/intent-attribution-agent-governance.md | Adds RFC2119 conformance norms, intent-policy schema documentation, and a policy enforcement audit section. |
| specs/github-mcp-access-control-compliance/tool-name-filter.yaml | New MCP access-control compliance fixture for allowed-tools filtering. |
| specs/github-mcp-access-control-compliance/README.md | Updates fixture list and adds an error-code reference table. |
| specs/github-mcp-access-control-compliance/empty-repos-block.yaml | New MCP access-control fixture for empty/absent repos deny behavior. |
| specs/github-mcp-access-control-compliance/combined-filter-allow.yaml | New MCP access-control fixture for combined-condition evaluation behavior. |
| specs/github-mcp-access-control-compliance/blocked-user-deny.yaml | New MCP access-control fixture for unconditional deny of blocked users. |
| pkg/intent/policy.go | Adds the new policy model, matching conditions, compiler, and merge logic. |
| pkg/intent/policy_test.go | Adds unit tests for rule precedence and “cannot weaken constraints” behavior. |
| pkg/cli/outcome_eval.go | Registers the new outcome evaluators in outcomeEvaluators. |
| pkg/cli/outcome_eval_workflow.go | Implements evalDispatchWorkflow and a stub evalUpdateDiscussion evaluator. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 15/15 changed files
- Comments generated: 4
- Review effort level: Low
| // Compile returns the most restrictive policy produced by merging all matching rules. | ||
| // The base is always the safe default policy. Rules are processed in order so that | ||
| // earlier (higher-precedence) rules cannot be weakened by later (lower-precedence) rules. | ||
| func (c *PolicyCompiler) Compile(record IntentRecord, repo RepositoryContext) ExecutionPolicy { | ||
| policy := safestDefaultPolicy() | ||
|
|
||
| for _, rule := range c.Rules { | ||
| if rule.When.Matches(record, repo) { | ||
| policy = mergePolicy(policy, rule.Set) | ||
| policy.RuleIDs = append(policy.RuleIDs, rule.ID) | ||
| } |
There was a problem hiding this comment.
Fixed in e2c15eb and 3315d2c. Compile() now seeds from the first matching rule's Set (not safestDefaultPolicy()) after sorting by scope precedence, so rules can express supervised/bounded autonomy, auto_merge_allowed=true, and max_attempts>1. applyFailClosedDefaults() fills in safe values only for fields no rule touched.
| // AllowedTools: if neither restricts tools, stay unrestricted. | ||
| // If the existing policy restricts, keep its restriction (higher-precedence wins). | ||
| // If only the incoming policy restricts, adopt that restriction. | ||
| // If both restrict, use the intersection (more restrictive). | ||
| if len(existing.AllowedTools) == 0 && len(incoming.AllowedTools) > 0 { | ||
| result.AllowedTools = slices.Clone(incoming.AllowedTools) | ||
| } else if len(existing.AllowedTools) > 0 && len(incoming.AllowedTools) > 0 { | ||
| var intersection []string | ||
| for _, tool := range existing.AllowedTools { | ||
| if slices.Contains(incoming.AllowedTools, tool) { | ||
| intersection = append(intersection, tool) | ||
| } | ||
| } | ||
| result.AllowedTools = intersection | ||
| } |
There was a problem hiding this comment.
Fixed in e2c15eb. mergePolicy now uses nil/[]string{} semantics — nil means unrestricted, []string{} (non-nil empty) means deny-all. When both sides restrict to non-overlapping sets, the intersection is []string{} (deny-all), not nil.
| // evalDispatchWorkflow checks whether a dispatched workflow run completed successfully. | ||
| // It looks for a run_id in the item metadata and queries the workflow run status. | ||
| // Spec: specs/safe-output-outcome-evaluation.md §20 | ||
| func evalDispatchWorkflow(item CreatedItemReport, repoOverride string) OutcomeReport { | ||
| repo := resolveItemRepo(item, repoOverride) | ||
| outcomeEvalWorkflowLog.Printf("Evaluating dispatch_workflow: repo=%s, url=%s", repo, item.URL) | ||
|
|
||
| report := OutcomeReport{ | ||
| Type: item.Type, | ||
| ObjectURL: item.URL, | ||
| Repo: repo, | ||
| } | ||
|
|
||
| // Extract run_id from metadata if available. | ||
| // JSON numbers unmarshal as float64; convert carefully to int64 to avoid | ||
| // precision loss for large GitHub run IDs (which can exceed 2^32). | ||
| var runID int64 | ||
| if item.Metadata != nil { | ||
| if v, ok := item.Metadata["run_id"]; ok { | ||
| switch id := v.(type) { | ||
| case float64: | ||
| if id > 0 && id <= math.MaxInt64 && id == math.Trunc(id) { | ||
| runID = int64(id) | ||
| } | ||
| case int: | ||
| runID = int64(id) | ||
| case int64: | ||
| runID = id | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if runID <= 0 { | ||
| // No run ID available — workflow may not have been dispatched or ID not captured | ||
| report.Result = OutcomePending | ||
| report.Detail = "no run ID available; dispatch may still be queued" | ||
| return report | ||
| } | ||
|
|
||
| data, err := workflowOutcomeGHAPIGet(fmt.Sprintf("actions/runs/%d", runID), repo) | ||
| if err != nil { | ||
| report.Result = OutcomeError | ||
| report.EvalError = err.Error() | ||
| return report | ||
| } | ||
|
|
||
| status, _ := data["status"].(string) | ||
| conclusion, _ := data["conclusion"].(string) | ||
| outcomeEvalWorkflowLog.Printf("dispatch_workflow run %d: status=%s, conclusion=%s", runID, status, conclusion) | ||
|
|
||
| switch { | ||
| case status == "completed" && conclusion == "success": | ||
| report.Result = OutcomeAccepted | ||
| report.Detail = "workflow run completed with success" | ||
| case status == "completed" && (conclusion == "failure" || conclusion == "timed_out" || conclusion == "cancelled"): | ||
| report.Result = OutcomeRejected | ||
| report.Detail = "workflow run completed with " + conclusion | ||
| case status == "completed": | ||
| report.Result = OutcomeIgnored | ||
| report.Detail = "workflow run completed with " + conclusion | ||
| default: | ||
| report.Result = OutcomePending | ||
| report.Detail = "workflow run status: " + status | ||
| } | ||
| return report |
There was a problem hiding this comment.
Fixed in e2c15eb. Added pkg/cli/outcome_eval_workflow_test.go with 9 tests covering: no run_id, zero run_id, API error, completed/success, completed/failure+timed_out+cancelled+action_required, completed/other conclusion, in_progress/pending, and float64 overflow guard.
| ## Error Code Reference | ||
|
|
||
| When `expected.decision` is `deny`, the fixture records the MCP JSON-RPC error code that the | ||
| implementation MUST return. The codes used in these fixtures are: | ||
|
|
||
| | Code | Denial Reason | | ||
| |---|---| | ||
| | `-32001` | Repository not on the allowlist (`repos` filter) | | ||
| | `-32002` | Label not on the allowlist (`allowed-labels` filter) | | ||
| | `-32003` | Label on the blocklist (`blocked-labels` filter) | | ||
| | `-32004` | User is blocked (`blocked-users` filter) | | ||
| | `-32005` | Tool name not permitted (`allowed-tools` filter) | | ||
| | `-32006` | Content integrity level below threshold (`min-integrity` filter) | | ||
| | `-32007` | Repository is private and `private-repos: false` | | ||
| | `-32008` | User role is insufficient (`role` filter) | |
There was a problem hiding this comment.
Fixed in e2c15eb. The error-code table now matches the actual spec and existing fixtures: -32001 repos, -32002 roles, -32003 private-repos, -32004 blocked-users, -32005 allowed-tools, -32006 min-integrity. Removed the incorrect -32007/-32008 entries.
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ PR Code Quality Reviewer completed the code quality review. |
🧪 Test Quality Sentinel Report✅ Test Quality Score: 100/100 — Excellent
📊 Metrics (3 tests)
Per-test classification:
Verdict
All tests verify genuine behavioral contracts from the policy precedence spec (intent-attribution-agent-governance.md). Each assertion carries descriptive failure context. No mocking violations. Build tags compliant. Excellent test quality — these tests will catch real precedence-logic bugs. Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"See Network Configuration for more information.
|
…ns merge Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (505 new lines in 📄 Draft ADR committed:
📋 What to do next
Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision. ❓ Why ADRs Matter
ADRs create a searchable, permanent record of why the codebase looks the way it does. Future contributors (and your future self) will thank you. 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
REQUEST_CHANGES — 3 correctness/security bugs in the new Go code must be fixed before merge.
### Blocking issues (3 critical/high)
-
pkg/intent/policy.go—AllowedToolsdisjoint intersection returns nil, silently granting unrestricted tool access (security). When two rules each restrictAllowedToolsto non-overlapping sets,mergePolicystores a nil slice, which downstreamlen == 0checks treat as "no restriction". Fix: initialiseintersectionto[]string{}(non-nil empty). -
pkg/cli/outcome_eval_workflow.go—float64 <= math.MaxInt64overflow guard is wrong (correctness).math.MaxInt64cannot be represented exactly as float64 and rounds up; a float64 at that boundary passes the guard butint64()overflows tomath.MinInt64, silently dropping real run IDs. Fix: cap at1<<53. -
pkg/cli/outcome_eval_workflow.go—evalUpdateDiscussionstub registered in live dispatch map (correctness/regression). Every realupdate_discussionitem will report perpetualOutcomePending, poisoning acceptance rates and preventing alerting. This is worse than the previousevalGenericStickyfallback. Fix: don't register it until GraphQL is implemented.
### Non-blocking (high/medium) — should address before GA
PolicyRule.Scopeis never read byCompile; the claimed org→repo→intent ordering is only a slice-ordering convention, not enforced. Tests pass only because test data happens to be ordered correctly.action_requiredworkflow conclusion is mapped toOutcomeIgnoredinstead ofOutcomePending— silently hides workflows awaiting environment protection approval.AutoMergeAllowed=falsezero-value fires for every rule that doesn't mention the field, over-constraining all higher-precedence rules.evalDispatchWorkflowhas no unit tests despite non-trivial float64/int64 conversion and multi-branch conclusion logic.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
proxy.golang.org
To allow these domains, add them to the
network.allowedlist in your workflow frontmatter:
network:
allowed:
- defaults
- "proxy.golang.org"See Network Configuration for more information.
🔎 Code quality review by PR Code Quality Reviewer · 159.8 AIC · ⌖ 7.18 AIC · ⊞ 5.4K
Comment /review to run again
| intersection = append(intersection, tool) | ||
| } | ||
| } | ||
| result.AllowedTools = intersection |
There was a problem hiding this comment.
Security: empty intersection is treated as unrestricted — two disjoint AllowedTools sets escalate to full tool access.
When existing.AllowedTools and incoming.AllowedTools are both non-empty but share no common tools, the intersection variable is never appended to and remains nil. result.AllowedTools = intersection stores a nil slice, and every downstream check (len(AllowedTools) == 0) treats nil as unrestricted. The policy that was supposed to deny all tools now permits every tool.
💡 Suggested fix
When both sides restrict tools but share nothing, the agent must be allowed zero tools. Use a non-nil empty slice as the sentinel:
} else if len(existing.AllowedTools) > 0 && len(incoming.AllowedTools) > 0 {
intersection := []string{} // non-nil empty = "no tools allowed"
for _, tool := range existing.AllowedTools {
if slices.Contains(incoming.AllowedTools, tool) {
intersection = append(intersection, tool)
}
}
result.AllowedTools = intersection
}All callers must consistently treat len(AllowedTools) == 0 && AllowedTools != nil as "deny all" and AllowedTools == nil as "unrestricted". Document this invariant at the ExecutionPolicy struct definition.
There was a problem hiding this comment.
Fixed in e2c15eb. mergePolicy now uses nil vs []string{} to distinguish unrestricted from deny-all. Disjoint AllowedTools intersection produces []string{} (deny-all), not nil (unrestricted). Covered by TestPolicyCompilerAllowedToolsDenyAllOnEmptyIntersection.
| if v, ok := item.Metadata["run_id"]; ok { | ||
| switch id := v.(type) { | ||
| case float64: | ||
| if id > 0 && id <= math.MaxInt64 && id == math.Trunc(id) { |
There was a problem hiding this comment.
Correctness: float64 <= math.MaxInt64 guard doesn't prevent int64 overflow for large GitHub run IDs.
math.MaxInt64 (9223372036854775807) cannot be represented exactly as float64; Go rounds it up to 9.223372036854776e18. A float64 ID equal to that rounded value passes the check (id <= math.MaxInt64 is true), but int64(id) overflows — on amd64 this yields math.MinInt64 (negative), so runID <= 0 and the function returns OutcomePending with "no run ID available", silently dropping a real dispatched run.
💡 Suggested fix
GitHub run IDs fit comfortably within float64's exact integer range (253 ≈ 9×1015). Use that as the upper bound:
const maxExactFloat64Int = 1 << 53 // 9007199254740992
case float64:
if id > 0 && id <= maxExactFloat64Int && id == math.Trunc(id) {
runID = int64(id)
}This is consistent with how the JSON spec itself handles integers and eliminates any possibility of overflow.
There was a problem hiding this comment.
Fixed in 3315d2c. The guard now uses maxSafeFloat64Int = 1 << 53 (2^53, the largest integer representable exactly as float64) instead of math.MaxInt64. Test TestEvalDispatchWorkflowFloat64OverflowGuard verifies values above 2^53 are rejected.
| return policy | ||
| } | ||
|
|
||
| // safestDefaultPolicy returns the most restrictive ExecutionPolicy baseline. |
There was a problem hiding this comment.
Correctness: PolicyRule.Scope is declared but never read — the claimed org→repo→intent precedence is only a convention, not enforced by code.
PolicyCompiler.Compile iterates c.Rules in declaration order without inspecting rule.Scope. The tests pass only because the test author happened to put org rules first in the slice. Any caller that inserts rules in a different order will silently get wrong precedence. The Scope field is misleading — it implies the compiler enforces ordering, but it doesn't.
💡 Options
Option A — Enforce scope ordering:
Before merging, sort rules by scope rank (organization=0, repository=1, intent=2, workflow=3) so declaration order doesn't matter:
func scopeRank(s string) int {
switch s {
case "organization": return 0
case "repository": return 1
case "intent": return 2
case "workflow": return 3
default: return 4
}
}
// stable sort before the merge loop
slices.SortStableFunc(rules, func(a, b PolicyRule) int {
return cmp.Compare(scopeRank(a.Scope), scopeRank(b.Scope))
})Option B — Remove the field if ordering by caller-provided slice position is intentional, and update the documentation and JSON schema accordingly.
There was a problem hiding this comment.
Fixed in 3315d2c. Compile() now sorts matched rules by scopePriorityOrder (org=4 > repo=3 > intent=2 > workflow=1 > unscoped=0) using slices.SortStableFunc before seeding the policy. TestPolicyCompilerScopeOrderingEnforced verifies rules declared in reverse order still compile with org values winning.
| case status == "completed" && conclusion == "success": | ||
| report.Result = OutcomeAccepted | ||
| report.Detail = "workflow run completed with success" | ||
| case status == "completed" && (conclusion == "failure" || conclusion == "timed_out" || conclusion == "cancelled"): |
There was a problem hiding this comment.
Correctness: action_required conclusion is mapped to OutcomeIgnored but it represents a blocking, non-terminal state.
The GitHub Actions API documents at least six non-success conclusions: failure, cancelled, timed_out, action_required, neutral, skipped. The code handles the first three as OutcomeRejected and success as OutcomeAccepted, but the catch-all case status == "completed" maps every other conclusion — including action_required — to OutcomeIgnored. A workflow awaiting environment protection approval or a required reviewer sits in completed/action_required; mapping it to Ignored makes the monitoring system think the workflow is finished and doesn't need attention, when human intervention is required.
💡 Suggested fix
case status == "completed" && conclusion == "action_required":
report.Result = OutcomePending
report.Detail = "workflow run requires a manual action to continue"
case status == "completed" && (conclusion == "skipped" || conclusion == "neutral"):
report.Result = OutcomeIgnored
report.Detail = "workflow run completed with " + conclusion
case status == "completed":
// Unknown future conclusion: treat as ignored but log clearly
report.Result = OutcomeIgnored
report.Detail = "workflow run completed with unrecognised conclusion: " + conclusionThere was a problem hiding this comment.
Fixed in 3315d2c. action_required is now mapped to OutcomeRejected (blocking, requires manual intervention — does not self-resolve), joining failure, timed_out, and cancelled. Covered by TestEvalDispatchWorkflowActionRequired.
| report.Result = OutcomePending | ||
| report.Detail = "workflow run status: " + status | ||
| } | ||
| return report |
There was a problem hiding this comment.
Maintainability: evalUpdateDiscussion is a permanent stub wired into the live evaluator dispatch map, silently poisoning outcome metrics.
This function always returns OutcomePending. Once wired into outcomeEvaluators, every real update_discussion item will be stuck in perpetual pending status, inflating the pending count, deflating acceptance rates, and never triggering retry or alert logic. Before this PR, update_discussion fell back to evalGenericSticky which at least provided a real existence check. This is a regression in evaluation fidelity.
💡 What to do instead
Do not register a stub evaluator in the production dispatch map. Pick one of:
- Don't register it yet: leave
update_discussionout ofoutcomeEvaluatorsand let it fall back toevalGenericStickyas before until the GraphQL implementation exists. - Register it under a feature flag:
if os.Getenv("GH_AW_EVAL_DISCUSSION") == "1" { outcomeEvaluators["update_discussion"] = evalUpdateDiscussion } - Use
OutcomeUnknown/OutcomeIgnoredwith a clear reason string if you want the spec compliance benefit without metrics pollution, and update the spec table accordingly.
Also add a TODO with a linked issue number so this doesn't silently persist.
There was a problem hiding this comment.
Fixed in 3315d2c. evalUpdateDiscussion now returns OutcomeIgnored (not OutcomePending) with detail "discussion update check requires GraphQL (not yet implemented); outcome is advisory only". OutcomeIgnored is excluded from acceptance/rejection rate calculations, so it doesn't poison metrics. The function remains registered so callers get an explicit advisory result rather than falling through to evalGenericSticky's OutcomeUnknown path.
| Result: OutcomePending, | ||
| Detail: "discussion update check requires GraphQL (not yet implemented)", | ||
| } | ||
| } |
There was a problem hiding this comment.
Test coverage: evalDispatchWorkflow has zero unit tests for its non-trivial logic.
This function has multiple independently testable paths — multi-type run_id extraction, float64→int64 conversion with overflow guard, an injectable HTTP call (workflowOutcomeGHAPIGet), and a 4-branch conclusion switch — none of which are covered by any test in pkg/cli. Compare with evalSubmitPullRequestReview or evalCloseDiscussion, which each have dedicated test files.
💡 Minimum test cases needed
run_idpresent asfloat64,int,int64run_idabsent (nil metadata)run_idpresent but not a valid type (unknown type → runID stays 0)- API returns error →
OutcomeError status="completed",conclusion="success"→OutcomeAcceptedstatus="completed",conclusion="failure"→OutcomeRejectedstatus="completed",conclusion="timed_out"→OutcomeRejectedstatus="completed",conclusion="cancelled"→OutcomeRejectedstatus="completed",conclusion="action_required"(see separate comment)status="in_progress"→OutcomePending
The workflowOutcomeGHAPIGet var already supports injection; use it to stub the API in tests.
There was a problem hiding this comment.
Fixed in e2c15eb. Added pkg/cli/outcome_eval_workflow_test.go with 9 tests exercising workflowOutcomeGHAPIGet via stub injection, covering all status/conclusion combinations, float64 type handling, and the overflow guard.
| } | ||
|
|
||
| // AutoMergeAllowed: false is more restrictive; use AND. | ||
| if !incoming.AutoMergeAllowed { |
There was a problem hiding this comment.
Correctness: AutoMergeAllowed=false zero-value silently over-constrains — every rule that doesn't mention auto-merge disables it.
The condition if !incoming.AutoMergeAllowed { result.AutoMergeAllowed = false } fires whenever the field is false, which is Go's zero value. A rule fragment that only wants to set AllowedTools without expressing any opinion on auto-merge will unconditionally lock AutoMergeAllowed = false for all higher-precedence rules. In the current test suite all rules explicitly set this field, so the bug is hidden, but any real rule omitting it will disable auto-merge globally.
💡 Suggested fix
Use a pointer or a wrapper type to distinguish "explicitly false" from "unset":
type TriBool int8
const (
TriUnset TriBool = iota
TriTrue
TriFalse
)
type ExecutionPolicy struct {
// ...
AutoMergeAllowed TriBool `json:"auto_merge_allowed,omitempty"`
}Or, simpler: use *bool and skip the merge if incoming.AutoMergeAllowed == nil.
The same risk applies to HumanApprovalRequired, but there defaulting to false is safe (it only grants less restriction), so it is less urgent.
There was a problem hiding this comment.
Fixed in 3315d2c. AutoMergeAllowed changed to *bool: nil = unset (no preference), false = explicit denial, true = explicit grant. mergePolicy only applies the AND step when at least one side is non-nil, so rules that don't mention AutoMergeAllowed no longer silently disable it.
| "ambiguous intent with no matching rules must produce max_attempts=1") | ||
| assert.Empty(t, policy.RuleIDs, | ||
| "no applied rule IDs should be recorded when no rules match") | ||
| } |
There was a problem hiding this comment.
Test gap: the AllowedTools disjoint-intersection bug path is not covered.
All three tests use rules with overlapping AllowedTools or rules that don't specify AllowedTools at all. The critical case — two rules each specifying a non-overlapping AllowedTools list — is untested. This is exactly the code path that returns a nil slice and silently grants full tool access (see comment on policy.go:185).
💡 Minimum test to add
func TestPolicyCompilerAllowedToolsDisjointIntersectionDeniesAllTools(t *testing.T) {
compiler := &intent.PolicyCompiler{
Rules: []intent.PolicyRule{
{
ID: "rule-a",
Set: intent.ExecutionPolicy{AllowedTools: []string{"read_issue"}},
},
{
ID: "rule-b",
Set: intent.ExecutionPolicy{AllowedTools: []string{"push_branch"}},
},
},
}
policy := compiler.Compile(intent.IntentRecord{}, intent.RepositoryContext{})
// Disjoint sets: no tool should be allowed, NOT unrestricted
assert.NotNil(t, policy.AllowedTools,
"disjoint AllowedTools intersection must be empty (not nil/unrestricted)")
assert.Empty(t, policy.AllowedTools,
"disjoint AllowedTools intersection must deny all tools")
}There was a problem hiding this comment.
Fixed in e2c15eb. Added TestPolicyCompilerAllowedToolsDenyAllOnEmptyIntersection which asserts disjoint AllowedTools sets produce a non-nil empty slice (deny-all), not nil (unrestricted).
Review Summary: SPDD 2026-07-05 Batch 20-24Requesting changes on 9 issues across correctness, test coverage, and spec completeness. One is a security-correctness bug. 🔴 Must-fix (correctness / security)
🟡 Should-fix (test coverage gaps)
🟡 Should-fix (design clarity)
@copilot please address the review comments above.
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design, /tdd, /diagnosing-bugs, and /grill-with-docs — requesting changes on correctness and coverage gaps.
📋 Key Themes & Highlights
Key Issues
- Security bug —
AllowedToolsempty intersection returnsnil(unrestricted) instead of an empty slice (deny-all). Disjoint rule sets silently grant full tool access. (policy.go:185) - Vacuous test assertions —
MaxAttempts <= 2passes trivially becausesafestDefaultPolicy()already returns 1. Tests don't verify what they claim. (policy_test.go:87,:157) PolicyRule.Scopeis a silent convention, not enforced. Mis-ordered rules compile without error and produce a weakened policy. (policy.go:53)evalUpdateDiscussionreturnsOutcomePendingpermanently — a structural limitation masquerading as a transient state, potentially causing infinite polling. (outcome_eval_workflow.go:90)- No tests for
evalDispatchWorkflow— theworkflowOutcomeGHAPIGetseam exists but is never exercised. (outcome_eval_workflow.go:16) PolicyCondition.Matchesflat-label collision is documented but not guarded at load time; a validation step would close this. (policy.go:71)- Combined-filter fixture under-covers T-GH-82 — role and visibility failure scenarios are missing. (
combined-filter-allow.yaml:16) - Advisory-only policy gap needs code-level surfacing —
Compile()doc should warn that onlyAutonomyis wired to runtime enforcement. (intent-attribution-agent-governance.md:910)
Positive Highlights
- ✅
evalDispatchWorkflowcorrectly handles float64→int64 precision for large GitHub run IDs — good defensive coding. - ✅
safestDefaultPolicy()provides a solid fail-closed baseline; the "more restrictive wins" merge design is sound. - ✅ Policy test naming is excellent: test names read as specifications, and Arrange/Act/Assert structure is clear throughout.
- ✅ RFC 2119 conformance section in the governance spec is a strong addition, especially the fail-closed norms and ambiguous-root handling.
- ✅
intent-policy.jsonschema documentation is thorough and well-organized.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 144.9 AIC · ⌖ 6.95 AIC · ⊞ 6.7K
Comment /matt to run again
| intersection = append(intersection, tool) | ||
| } | ||
| } | ||
| result.AllowedTools = intersection |
There was a problem hiding this comment.
[/codebase-design] Empty AllowedTools intersection reverts to unrestricted instead of deny-all, inverting the security posture.
When two rules have disjoint tool sets, intersection is nil. Setting result.AllowedTools = nil makes the policy look like "allow any tool" — the exact opposite of what two explicit restrictions should produce.
💡 Suggested fix + regression test
Use make([]string, 0) to produce a non-nil empty slice so the deny-all intent is preserved:
intersection := make([]string, 0) // non-nil: empty == deny-all, not unrestricted
for _, tool := range existing.AllowedTools {
if slices.Contains(incoming.AllowedTools, tool) {
intersection = append(intersection, tool)
}
}
result.AllowedTools = intersectionAdd a test to lock this in:
func TestPolicyCompilerDisjointAllowedToolsDeniesAll(t *testing.T) {
compiler := &intent.PolicyCompiler{
Rules: []intent.PolicyRule{
{ID: "r1", When: intent.PolicyCondition{}, Set: intent.ExecutionPolicy{
AllowedTools: []string{"issue_read"},
}},
{ID: "r2", When: intent.PolicyCondition{}, Set: intent.ExecutionPolicy{
AllowedTools: []string{"create_pr"},
}},
},
}
policy := compiler.Compile(intent.IntentRecord{}, intent.RepositoryContext{})
require.NotNil(t, policy.AllowedTools,
"disjoint AllowedTools must produce non-nil empty slice (deny-all), not nil (unrestricted)")
assert.Empty(t, policy.AllowedTools)
}@copilot please address this.
There was a problem hiding this comment.
Fixed in e2c15eb. Intersection now initialises to nil (unrestricted when neither side restricts), adopts restriction when one side restricts, and produces []string{} (deny-all) for disjoint sets. Covered by TestPolicyCompilerAllowedToolsDenyAllOnEmptyIntersection.
| if incoming.MaxAttempts > 0 { | ||
| if result.MaxAttempts == 0 || incoming.MaxAttempts < result.MaxAttempts { | ||
| result.MaxAttempts = incoming.MaxAttempts | ||
| } |
There was a problem hiding this comment.
[/codebase-design] safestDefaultPolicy() sets MaxAttempts: 1, so an org rule with MaxAttempts: 2 is silently ignored — the min-wins logic always prefers the safe default.
The test on line 87 asserts <= 2, which passes trivially because the default (1) is already lower. This means the org rule's intent (allow up to 2 retries) is never applied and cannot be tested.
💡 Discussion
Two design options:
-
Zero means unset: treat
MaxAttempts: 0insafestDefaultPolicy()as "not yet decided", and only enforce a hard ceiling via a separatemaxAttemptsHardCapconstant. Rules can then raise from 0 to their desired value, and the min-wins merge correctly limits escalation. -
Always validate in isolation: document explicitly that
safestDefaultPolicy()already imposesMaxAttempts: 1and that rules cannot raise it beyond 1 at the global baseline — and update the test to assert== 1(not<= 2) so the constraint is visible.
Option 1 is more flexible; option 2 is simpler and may be intentional. Either way, the current test does not distinguish these paths, so the behavior should be made explicit.
@copilot please address this.
There was a problem hiding this comment.
Fixed in e2c15eb and 3315d2c. Compile() seeds from the first matching rule (after scope-priority sort), not from safestDefaultPolicy(). An org rule with MaxAttempts: 2 is now used as the baseline; subsequent lower-precedence rules can only lower it. TestPolicyCompilerRulesCanGrantLessRestrictiveThanSafeDefault verifies this.
| // the Domain/Priority/Risk fields each check for the presence of their value anywhere in that | ||
| // slice. Callers must ensure label values are unique across dimensions (e.g. no priority value | ||
| // that could collide with a domain value) to avoid false positives. | ||
| func (c PolicyCondition) Matches(record IntentRecord, repo RepositoryContext) bool { |
There was a problem hiding this comment.
[/codebase-design] PolicyCondition.Matches uses cross-dimension label search — a label value shared between dimensions (e.g., "critical" as both a risk level and a priority level) will cause rule mis-fires.
The comment documents the caveat but puts the enforcement burden entirely on the caller. In a security-sensitive policy compiler, this should be a hard invariant enforced at rule-load time, not a documentation note.
💡 Suggested improvement
Add a Validate() method on PolicyCompiler (or validate in a constructor) that checks label uniqueness across dimensions, returning an error for collisions:
func (c *PolicyCompiler) Validate(labels map[string]LabelDescriptor) error {
// Check that no label value appears in more than one dimension.
valueToEm := map[string]string{}
for name, desc := range labels {
key := desc.Value
if existing, ok := valueToEm[key]; ok {
return fmt.Errorf(
"label %q and %q share value %q across dimensions; PolicyCondition.Matches would be ambiguous",
existing, name, key,
)
}
valueToEm[key] = name
}
return nil
}Alternatively, store labels keyed by dimension (e.g., record.DomainLabels, record.PriorityLabels) so each Matches check is dimension-scoped without relying on uniqueness guarantees.
@copilot please address this.
There was a problem hiding this comment.
Acknowledged. The flat-label caveat is documented in the Matches godoc comment. A load-time validation step is tracked as a follow-up — beyond the scope of this batch. Callers must ensure label values are unique across dimensions.
| Repo: resolveItemRepo(item, repoOverride), | ||
| Result: OutcomePending, | ||
| Detail: "discussion update check requires GraphQL (not yet implemented)", | ||
| } |
There was a problem hiding this comment.
[/diagnosing-bugs] evalUpdateDiscussion permanently returns OutcomePending, which will cause the outcome evaluator to retry indefinitely with no eventual resolution.
A permanently-pending evaluator degrades into a hidden infinite wait: callers that poll until non-pending will loop forever, and the "partial" status in the spec table is now misleading — it behaves identically to the old evalGenericSticky fallback.
💡 Suggested improvement
Until GraphQL is implemented, return OutcomeIgnored with a clear Detail message, or add a MaxPollDuration guard in the evaluation loop. OutcomePending should only be used for genuinely transient states, not structural limitations:
func evalUpdateDiscussion(item CreatedItemReport, repoOverride string) OutcomeReport {
return OutcomeReport{
Type: item.Type,
ObjectURL: item.URL,
Repo: resolveItemRepo(item, repoOverride),
Result: OutcomeIgnored, // Not OutcomePending — this is a structural gap, not a transient state
Detail: "discussion update evaluation requires GraphQL (not yet implemented; treated as ignored)",
}
}Alternatively, add a TODO(graphql): #<tracking-issue> comment and a test that asserts the result is OutcomeIgnored so any future change is clearly visible.
@copilot please address this.
There was a problem hiding this comment.
Fixed in 3315d2c. evalUpdateDiscussion now returns OutcomeIgnored with an explicit detail message. OutcomeIgnored is excluded from acceptance/rejection rate calculations, preventing infinite retry and metric poisoning.
| // evalDispatchWorkflow checks whether a dispatched workflow run completed successfully. | ||
| // It looks for a run_id in the item metadata and queries the workflow run status. | ||
| // Spec: specs/safe-output-outcome-evaluation.md §20 | ||
| func evalDispatchWorkflow(item CreatedItemReport, repoOverride string) OutcomeReport { |
There was a problem hiding this comment.
[/tdd] evalDispatchWorkflow has no accompanying tests — the workflowOutcomeGHAPIGet seam is defined but never exercised.
Given the float64→int64 conversion logic and the 5-case status/conclusion switch, there are at least 8 meaningful test scenarios (nil metadata, run_id=0, API error, each conclusion branch, unknown status). Without tests, the next person to touch this code has no safety net.
💡 Minimum test coverage suggestions
func TestEvalDispatchWorkflow_NoRunID_ReturnsPending(t *testing.T) { ... }
func TestEvalDispatchWorkflow_APIError_ReturnsOutcomeError(t *testing.T) { ... }
func TestEvalDispatchWorkflow_Success_ReturnsAccepted(t *testing.T) { ... }
func TestEvalDispatchWorkflow_Failure_ReturnsRejected(t *testing.T) { ... }
func TestEvalDispatchWorkflow_Cancelled_ReturnsRejected(t *testing.T) { ... }
func TestEvalDispatchWorkflow_UnknownConclusion_ReturnsIgnored(t *testing.T) { ... }
func TestEvalDispatchWorkflow_InProgress_ReturnsPending(t *testing.T) { ... }
func TestEvalDispatchWorkflow_Float64RunID_NoPrecisionLoss(t *testing.T) { ... }Use workflowOutcomeGHAPIGet as the injection point (same pattern as ghAPIGet stubs in other outcome_eval_*.go test files).
@copilot please address this.
There was a problem hiding this comment.
Fixed in e2c15eb. pkg/cli/outcome_eval_workflow_test.go exercises workflowOutcomeGHAPIGet via the package-level var seam across 9 test cases covering all code paths.
| | `MaxAttempts` | **Not wired** | Not enforced at the orchestrator level. | | ||
| | `RuleIDs` | **Provenance only** | Recorded in the policy for auditing; not used to gate execution. | | ||
|
|
||
| **Risk**: Policy constraints defined in `.github/intent-policy.json` (or the equivalent `rules` array) have no runtime effect until the orchestrator is wired to call `AuthorizeTool` and enforce `WriteScope`, `HumanApprovalRequired`, and `RequiredChecks`. Any policy compiled by `PolicyCompiler.Compile()` today is purely advisory. |
There was a problem hiding this comment.
[/grill-with-docs] The spec acknowledges that all ExecutionPolicy fields except Autonomy are unconnected to the runtime — but the new PolicyCompiler Go code has no corresponding guard, warning, or feature flag to prevent it from silently providing false security assurance.
Shipping a PolicyCompiler that compiles valid-looking policies while enforcing nothing is a correctness hazard: callers might depend on DeniedTools or HumanApprovalRequired without realizing those constraints have no runtime effect.
💡 Options to surface the gap at the code level
- Add a
//nolint:unused // policy enforcement not yet wiredcomment onExecutionPolicy— forces explicit acknowledgment on each use. - Add a runtime-advisory note in
Compile()doc:// Compile returns a policy for the given intent. // WARNING: As of this implementation, only Autonomy is wired to runtime enforcement. // DeniedTools, AllowedTools, WriteScope, HumanApprovalRequired, AutoMergeAllowed, // RequiredChecks, and MaxAttempts are advisory only. // See specs/intent-attribution-agent-governance.md §AuthorizeTool Audit for status.
- Add a
Featurefield or wrapper so callers can check enforcement status before acting on compiled policies.
Option 2 is the least effort and makes the limitation visible to any future consumer.
@copilot please address this.
There was a problem hiding this comment.
Fixed in 3315d2c. ExecutionPolicy and PolicyCompiler doc comments now carry an explicit WARNING: advisory only note. The governance spec documents all unconnected fields as "not yet wired to runtime enforcement".
| "organization required check 'security-scan' must not be removed by repository rule") | ||
|
|
||
| // MaxAttempts must not be increased beyond the org limit. | ||
| assert.LessOrEqual(t, policy.MaxAttempts, 2, |
There was a problem hiding this comment.
[/tdd] assert.LessOrEqual(t, policy.MaxAttempts, 2) is a vacuous test — safestDefaultPolicy() starts at MaxAttempts: 1, which is always <= 2.
The assertion passes regardless of whether the org rule's MaxAttempts: 2 was actually applied or ignored. It gives no signal about whether the merge behavior is correct.
💡 Sharper assertion
If the intent is that MaxAttempts cannot be raised above the safe default:
assert.Equal(t, 1, policy.MaxAttempts,
"org rule MaxAttempts=2 must be capped by the safe default of 1 — cannot raise above baseline")If the intent is that the org rule's value (2) overrides the default (1) and the repo rule (10) cannot raise it further:
assert.Equal(t, 2, policy.MaxAttempts,
"org rule MaxAttempts=2 must be applied; repo rule's MaxAttempts=10 must not increase it")Either way, assert.Equal makes the intended invariant explicit and forces the test to fail if the merge logic changes.
@copilot please address this.
There was a problem hiding this comment.
Fixed in 3315d2c. The assertion was changed from LessOrEqual(t, policy.MaxAttempts, 2) to Equal(t, 2, policy.MaxAttempts) so it verifies the actual compiled value, not a vacuous upper bound.
| spec_refs: | ||
| - "§10 — Combined conditions require all to be satisfied for allow" | ||
| - "§10.1 — T-GH-081: request satisfying all conditions is allowed" | ||
| - "§10.2 — T-GH-082: request failing any single condition is denied" |
There was a problem hiding this comment.
[/tdd] T-GH-82 claims "failure of any single condition MUST produce a deny" but only two of the four configured conditions (repo, integrity) are tested in failure scenarios — role failure and private-repos visibility failure are not covered.
This leaves §10 combined-condition evaluation under-specified: an implementation could fail to check role or visibility in the combined path and all three fixture scenarios would still pass.
💡 Suggested additional scenarios
Add scenario_id: combined-filter-allow-D (role fails → denied with -32008) and scenario_id: combined-filter-allow-E (visibility fails → denied with -32007) to make T-GH-82 complete:
# --- Scenario D: role condition fails → denied ---
- scenario_id: "combined-filter-allow-D"
description: "Request with insufficient role is denied even when repo, integrity, and visibility match"
input:
tool_config:
repos: ["github/gh-aw"]
roles: ["write"]
private-repos: true
min-integrity: "approved"
request:
repository: "github/gh-aw"
user_role: "read"
is_private: true
content_integrity: "approved"
expected:
decision: deny
error_code: -32008
reason: "user role is insufficient"@copilot please address this.
There was a problem hiding this comment.
Fixed in 3315d2c. Added scenarios D (role failure → -32002) and E (private-repo visibility failure → -32003) to combined-filter-allow.yaml, completing T-GH-82's "failure of any single condition MUST deny" coverage.
| // PolicyRule pairs a match condition with a policy fragment to apply. | ||
| type PolicyRule struct { | ||
| ID string `json:"id"` | ||
| Scope string `json:"scope,omitempty"` // "organization", "repository", "intent", or "workflow" |
There was a problem hiding this comment.
[/codebase-design] PolicyRule.Scope is documented as a "hint for evaluation ordering" but has no effect on rule application — Compile() processes rules in declaration order regardless of scope value.
The spec and the intent-policy.json schema both state "Rules MUST be listed from highest to lowest precedence", which places the ordering burden entirely on the JSON author. A mis-ordered policy file (e.g., intent rule before organization rule) compiles silently and produces a weakened policy that passes all tests.
💡 Suggested improvement
Validate or sort by scope rank in Compile() (or a separate Validate() step) before processing:
var scopeOrder = map[string]int{
"organization": 0,
"repository": 1,
"intent": 2,
"workflow": 3,
"": 4, // unset = lowest precedence
}
// In Compile(), add:
for i := 1; i < len(c.Rules); i++ {
if scopeOrder[c.Rules[i-1].Scope] > scopeOrder[c.Rules[i].Scope] {
return ExecutionPolicy{}, fmt.Errorf(
"rule %q (%s) must appear before rule %q (%s)",
c.Rules[i].ID, c.Rules[i].Scope,
c.Rules[i-1].ID, c.Rules[i-1].Scope,
)
}
}At a minimum, add a test that demonstrates mis-ordered rules do or do not produce different compiled output, to document the intended behavior.
@copilot please address this.
There was a problem hiding this comment.
Fixed in 3315d2c. Compile() now explicitly sorts matched rules by scopePriorityOrder using slices.SortStableFunc, enforcing org > repo > intent > workflow precedence regardless of declaration order. TestPolicyCompilerScopeOrderingEnforced verifies the invariant.
🤖 PR Triage — Run §28750744185
Score breakdown: Impact 38 + Urgency 19 + Quality 14 Rationale: Large SPDD batch (16 files, +1282/-6) adding Go outcome evaluators, a new PolicyCompiler, and spec coverage. CI passing (4/5 SUCCESS). High priority: fills P0/P1/P2 gaps identified in the plan. High risk due to scope (new interfaces, API integration). Bot reviewer flagged concerns; a dismissed review from github-actions should be followed up. Needs human validation of evaluator logic and PolicyCompiler correctness. Recommended next step: Human fast-track review — focus on evaluator edge cases and PolicyCompiler interface contracts.
|
|
@copilot please run the
|
…, AllowedTools deny-all, workflow evaluator tests, error code table Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
…mment, improve AllowedTools switch comment, combine deny-all assertion, annotate Labels usage Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Done. Addressed all four unresolved review threads from
Commits: CI note: |
|
@copilot please run the
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
…ns, scope ordering, AutoMergeAllowed *bool, tests Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
…est variable Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Done. All non-outdated review threads from the first and second passes have been addressed. Here's a summary of the fixes pushed in commits
Pre-existing failure: |
|
@copilot please run the Unresolved review feedback to address:
Run: https://github.com/github/gh-aw/actions/runs/28754210553
|
Done. Here is the maintainer-ready summary:
Actions taken this run: Replied to all 21 unresolved review threads; no code changes were needed (all fixes were already committed). Hand-off: CI must be re-triggered by a maintainer (close/reopen PR, Still needed: Human approval to merge. |
Addresses the P0/P1/P2 gaps identified in the batch 20-24 SPDD plan: missing Go evaluators falling back to
evalGenericSticky, unimplementedPolicyCompiler, and spec coverage holes across OTel, replace-label, MCP fixtures, and intent-attribution.Go: outcome evaluators (
pkg/cli/)evalDispatchWorkflow: queriesactions/runs/{run_id}via GitHub API; mapsstatus/conclusiontoOutcomeAccepted/OutcomeRejected/OutcomePending/OutcomeIgnored. Usesint64for run IDs (notfloat64) to avoid precision loss above 2⁵³.evalUpdateDiscussion: pending stub; full evaluation requires GraphQL (same pattern asevalCloseDiscussion).outcomeEvaluatorsmap.Go: policy compiler (
pkg/intent/)ExecutionPolicy,PolicyRule,PolicyCondition,PolicyCompilertypes.mergePolicy()implements more-restrictive-wins:DeniedTools/RequiredChecksunion,AllowedToolsintersection,HumanApprovalRequiredOR,AutoMergeAllowedAND,MaxAttemptsmin.PolicyCondition.Matchesdocuments the flat-label dimension caveat.Spec:
safe-output-outcome-evaluation.mddispatch_workflowandupdate_discussionrows to reference new evaluators.replace_labelto both the Default Acceptance Map and Implementation Status tables.Spec:
otel-observability-spec.md§17Spec:
intent-attribution-agent-governance.md.github/intent-policy.jsonschema documentation with field tables and full example.Authorizer.AuthorizeToolimplementation audit: documents allExecutionPolicyfields as not yet wired to runtime enforcement — policy today is advisory only.Compliance fixtures
specs/replace-label-compliance/(new):rl-001-glob-semantics.yamlandrl-003-blocklist-ordering.yaml, 4 scenarios each.specs/github-mcp-access-control-compliance/: 4 new fixture stubs covering §5.3 (empty repos), §6.1 (tool-name filter), §7.2 (blocked users), §10 (combined filters). README gains error code reference table (-32001 through -32008).