Skip to content
2 changes: 2 additions & 0 deletions pkg/cli/outcome_eval.go
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,8 @@ var outcomeEvaluators = map[string]outcomeEvaluator{
"push_to_pull_request_branch": evalPushToPRBranch,
"add_reviewer": evalAddReviewer,
"submit_pull_request_review": evalSubmitPullRequestReview,
"dispatch_workflow": evalDispatchWorkflow,
"update_discussion": evalUpdateDiscussion,
}

// EvaluateOutcomes checks the current state of all safe output items from a run.
Expand Down
91 changes: 91 additions & 0 deletions pkg/cli/outcome_eval_workflow.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,91 @@
package cli

import (
"fmt"
"math"

"github.com/github/gh-aw/pkg/logger"
)

var outcomeEvalWorkflowLog = logger.New("cli:outcome_eval_workflow")
var workflowOutcomeGHAPIGet = ghAPIGet

// 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 {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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"):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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: " + conclusion

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 = 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
Comment on lines +18 to +88

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. Don't register it yet: leave update_discussion out of outcomeEvaluators and let it fall back to evalGenericSticky as before until the GraphQL implementation exists.
  2. Register it under a feature flag: if os.Getenv("GH_AW_EVAL_DISCUSSION") == "1" { outcomeEvaluators["update_discussion"] = evalUpdateDiscussion }
  3. Use OutcomeUnknown/OutcomeIgnored with 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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

// evalUpdateDiscussion checks whether a discussion edit stuck.
// Full evaluation requires GraphQL; this evaluator returns pending until implemented.
// Spec: specs/safe-output-outcome-evaluation.md §12
func evalUpdateDiscussion(item CreatedItemReport, repoOverride string) OutcomeReport {
return OutcomeReport{
Type: item.Type,
ObjectURL: item.URL,
Repo: resolveItemRepo(item, repoOverride),
Result: OutcomePending,
Detail: "discussion update check requires GraphQL (not yet implemented)",
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_id present as float64, int, int64
  • run_id absent (nil metadata)
  • run_id present but not a valid type (unknown type → runID stays 0)
  • API returns error → OutcomeError
  • status="completed", conclusion="success" → OutcomeAccepted
  • status="completed", conclusion="failure" → OutcomeRejected
  • status="completed", conclusion="timed_out" → OutcomeRejected
  • status="completed", conclusion="cancelled" → OutcomeRejected
  • status="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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

190 changes: 190 additions & 0 deletions pkg/intent/policy.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,190 @@
package intent

import "slices"

// autonomyOrder maps autonomy level names to their restrictiveness rank.
// Higher rank means more restrictive.
var autonomyOrder = map[string]int{
"propose_only": 3,
"supervised": 2,
"bounded": 1,
"": 0,
}

// writeScopeOrder maps write_scope values to their restrictiveness rank.
// Higher rank means more restrictive.
var writeScopeOrder = map[string]int{
"none": 3,
"feature_branch": 2,
"any_branch": 1,
"": 0,
}

// ExecutionPolicy governs what an agent may do for a given intent.
type ExecutionPolicy struct {
Autonomy string `json:"autonomy"`

AllowedTools []string `json:"allowed_tools,omitempty"`
DeniedTools []string `json:"denied_tools,omitempty"`

WriteScope string `json:"write_scope"`

RequiredChecks []string `json:"required_checks,omitempty"`

HumanApprovalRequired bool `json:"human_approval_required"`
AutoMergeAllowed bool `json:"auto_merge_allowed"`

MaxAttempts int `json:"max_attempts"`

RuleIDs []string `json:"rule_ids,omitempty"`
}

// RepositoryContext carries repository-level context used when matching policy rules.
type RepositoryContext struct {
Owner string `json:"owner,omitempty"`
Name string `json:"name,omitempty"`
Visibility string `json:"visibility,omitempty"` // "public" or "private"
Org string `json:"org,omitempty"`
}

// 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"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

When PolicyCondition `json:"when"`
Set ExecutionPolicy `json:"set"`
}

// PolicyCondition describes when a rule applies.
type PolicyCondition struct {
Domain string `json:"domain,omitempty"`
Priority string `json:"priority,omitempty"`
Risk string `json:"risk,omitempty"`
Org string `json:"org,omitempty"`
}

// Matches returns true when the condition is satisfied by the given intent and repository.
// Labels are matched as flat strings. If a record carries labels like ["security","p1","critical"],
// 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 {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if c.Domain != "" && !slices.Contains(record.Labels, c.Domain) {
return false
}
if c.Priority != "" && !slices.Contains(record.Labels, c.Priority) {
return false
}
if c.Risk != "" && !slices.Contains(record.Labels, c.Risk) {
return false
}
if c.Org != "" && c.Org != repo.Org && c.Org != repo.Owner {
return false
}
return true
}

// PolicyCompiler compiles a set of rules into an ExecutionPolicy for a given intent.
// Rules are applied in declaration order; each merged step preserves stricter
// higher-precedence constraints.
type PolicyCompiler struct {
Rules []PolicyRule
}

// 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)
}
Comment on lines +126 to +139

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

return policy
}

// safestDefaultPolicy returns the most restrictive ExecutionPolicy baseline.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// Unknown or ambiguous intent must not grant elevated authority.
func safestDefaultPolicy() ExecutionPolicy {
return ExecutionPolicy{
Autonomy: "propose_only",
WriteScope: "none",
HumanApprovalRequired: true,
AutoMergeAllowed: false,
MaxAttempts: 1,
}
}

// mergePolicy merges a new policy fragment into an existing accumulated policy,
// always preserving the more restrictive value for each field.
//
// The existing policy represents already-accumulated (higher-precedence) constraints.
// The incoming fragment represents a lower-precedence rule's desired settings.
// The result must never be less restrictive than the existing policy.
func mergePolicy(existing, incoming ExecutionPolicy) ExecutionPolicy {
result := existing

// Autonomy: keep the more restrictive level.
if autonomyOrder[incoming.Autonomy] > autonomyOrder[existing.Autonomy] {
result.Autonomy = incoming.Autonomy
}

// WriteScope: keep the more restrictive scope.
if writeScopeOrder[incoming.WriteScope] > writeScopeOrder[existing.WriteScope] {
result.WriteScope = incoming.WriteScope
}

// HumanApprovalRequired: true is more restrictive; use OR.
if incoming.HumanApprovalRequired {
result.HumanApprovalRequired = true
}

// AutoMergeAllowed: false is more restrictive; use AND.
if !incoming.AutoMergeAllowed {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

result.AutoMergeAllowed = false
}

// MaxAttempts: lower is more restrictive; keep the minimum of both if both are set.
if incoming.MaxAttempts > 0 {
if result.MaxAttempts == 0 || incoming.MaxAttempts < result.MaxAttempts {
result.MaxAttempts = incoming.MaxAttempts
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/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:

  1. Zero means unset: treat MaxAttempts: 0 in safestDefaultPolicy() as "not yet decided", and only enforce a hard ceiling via a separate maxAttemptsHardCap constant. Rules can then raise from 0 to their desired value, and the min-wins merge correctly limits escalation.

  2. Always validate in isolation: document explicitly that safestDefaultPolicy() already imposes MaxAttempts: 1 and 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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

// RequiredChecks: union of both lists (adding checks is always more restrictive).
for _, check := range incoming.RequiredChecks {
if !slices.Contains(result.RequiredChecks, check) {
result.RequiredChecks = append(result.RequiredChecks, check)
}
}

// DeniedTools: union of both lists (denying more tools is always more restrictive).
for _, tool := range incoming.DeniedTools {
if !slices.Contains(result.DeniedTools, tool) {
result.DeniedTools = append(result.DeniedTools, tool)
}
}

// 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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/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 = intersection

Add 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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// If only existing restricts, result.AllowedTools already has the existing value.

return result
}
Loading
Loading