From 1a96f02ce43722974abf267cb97f7ca35238b34c Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 21 Aug 2026 16:08:44 +0000 Subject: [PATCH 1/4] Initial plan From 3f799b1a8d17d6d00a3daebc3aa9f106e06681e5 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 21 Aug 2026 16:17:43 +0000 Subject: [PATCH 2/4] Harden SPDD specification coverage Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- ...config_conformance_registry_formal_test.go | 6 +++--- .../awf_config_safeguards_formal_test.go | 6 +++--- ...github-mcp-access-control-specification.md | 6 ++++-- scratchpad/guard-policies-specification.md | 13 +++++++++++-- scratchpad/safe-outputs-specification.md | 19 +++++++++++++++++++ specs/awf-config-sources-compliance/README.md | 3 ++- specs/awf-config-sources-spec.md | 12 ++++++------ 7 files changed, 48 insertions(+), 17 deletions(-) diff --git a/pkg/workflow/awf_config_conformance_registry_formal_test.go b/pkg/workflow/awf_config_conformance_registry_formal_test.go index 17b8b562245..c3df10e2a7f 100644 --- a/pkg/workflow/awf_config_conformance_registry_formal_test.go +++ b/pkg/workflow/awf_config_conformance_registry_formal_test.go @@ -167,11 +167,11 @@ func formalConformanceRegistrySeriesDisjoint(id string) bool { func TestFormalConformanceRegistry_P1_TestIDMonotonicity(t *testing.T) { next := formalConformanceRegistryNextPlainID(formalConformanceRegistryBaselineRows(t)) - assert.Equal(t, "T-DR-011", next) + assert.Equal(t, "T-DR-012", next) nextValue, ok := formalConformanceRegistryParsePlainID(next) require.True(t, ok) - assert.Equal(t, 11, nextValue) + assert.Equal(t, 12, nextValue) } func TestFormalConformanceRegistry_P1_EmptyRegistryStartsAtOne(t *testing.T) { @@ -228,7 +228,7 @@ func TestFormalConformanceRegistry_P8_SpecCrossReferenceRequired(t *testing.T) { assert.True(t, formalConformanceRegistryHasSpecCrossReference(specContent, row.TestID), row.TestID) } - assert.False(t, formalConformanceRegistryHasSpecCrossReference(specContent, "T-DR-011")) + assert.False(t, formalConformanceRegistryHasSpecCrossReference(specContent, "T-DR-012")) } func TestFormalConformanceRegistry_P9_DriftSeriesVsSafeguardSeriesDisjoint(t *testing.T) { diff --git a/pkg/workflow/awf_config_safeguards_formal_test.go b/pkg/workflow/awf_config_safeguards_formal_test.go index 410d3fe4bf3..09a97a5bd0e 100644 --- a/pkg/workflow/awf_config_safeguards_formal_test.go +++ b/pkg/workflow/awf_config_safeguards_formal_test.go @@ -105,18 +105,18 @@ func TestAWFConfigSafeguard_TDRSAFE001_SnapshotStoragePathSelection(t *testing.T assert.NotEqual(t, filepath.Clean(formalSnapshotStoragePath(false)), filepath.Clean(formalSnapshotStoragePath(true))) } -func TestFormalP13_EscalationOwnerAssignmentFallbackChain(t *testing.T) { +func TestAWFConfigSafeguard_TDR011_EscalationOwnerAssignmentFallbackChain(t *testing.T) { assert.Equal(t, "@last-maintainer", formalEscalationOwner("@last-maintainer", "@on-call")) assert.Equal(t, "@on-call", formalEscalationOwner("", "@on-call")) } -func TestFormalP14_EscalationOwnerMustNotBeUnassigned(t *testing.T) { +func TestAWFConfigSafeguard_TDR011_EscalationOwnerMustNotBeUnassigned(t *testing.T) { assert.True(t, formalEscalationOwnerNonEmpty(formalEscalationOwner("@last-maintainer", "@on-call"))) assert.True(t, formalEscalationOwnerNonEmpty(formalEscalationOwner("", "@on-call"))) assert.False(t, formalEscalationOwnerNonEmpty(formalEscalationOwner("", ""))) } -func TestFormalP15_EscalationAcknowledgementWindow(t *testing.T) { +func TestAWFConfigSafeguard_TDR011_EscalationAcknowledgementWindow(t *testing.T) { assignedFriday := time.Date(2026, 8, 7, 9, 0, 0, 0, time.UTC) mondayDeadline := time.Date(2026, 8, 10, 9, 0, 0, 0, time.UTC) diff --git a/scratchpad/github-mcp-access-control-specification.md b/scratchpad/github-mcp-access-control-specification.md index 256199da849..1a8adf19d12 100644 --- a/scratchpad/github-mcp-access-control-specification.md +++ b/scratchpad/github-mcp-access-control-specification.md @@ -1026,11 +1026,11 @@ approval-labels: [true] # All elements must be strings **Validation**: ```yaml # INVALID -repos: [] # Empty array blocks all access +repos: [] # Empty allowlist would block all access; omit `repos` for no repository restriction roles: [] # Empty array blocks all access # VALID -# (omit field entirely if you want no restrictions) +# Omitted `repos` means no repository restriction; an empty array is rejected to prevent accidental deny-all. repos: - "*/*" # Explicit all-access ``` @@ -1852,6 +1852,7 @@ This subsection specifies normative failure-mode behavior for the case where gua - Implementations MUST fail closed on malformed guard-policy configuration: until the configuration error is fixed, the affected GitHub MCP tools MUST NOT be enabled, rather than falling back to an unrestricted ("all repos", `min-integrity: none`) policy. - Compilation error messages for malformed guard-policy configuration SHOULD identify the offending field, the value received, and the set of valid values or formats, so the misconfiguration can be corrected without consulting this specification. - Implementations MUST NOT partially apply a malformed guard policy (for example, enforcing `min-integrity` while ignoring an invalid `allowed-repos` value); validation MUST treat the guard policy as a single unit that either fully passes validation or causes compilation to fail. +- When a username appears in both `blocked-users` and `trusted-users`, the blocked-user decision MUST take precedence and the item MUST be denied. This deterministic rule prevents a trusted-user grant from weakening an explicit deny list (T-GH-094). ### 9.7 Open Questions @@ -2124,6 +2125,7 @@ Additional blocked-user validation tests in `TestValidateGitHubGuardPolicy`: - **T-GH-091**: When both `blocked-users` and `min-integrity` are configured, access is the conjunction of P5_NotBlocked AND P6_IntegrityMet; a non-blocked user with content at or above the threshold is allowed - **T-GH-092**: Non-blocked user with content exceeding the configured threshold is allowed; P5 and P6 both pass - **T-GH-093**: Blocked user whose content also fails the integrity threshold is denied with `-32005` (P5_NotBlocked fires before P6_IntegrityMet per §8.5 combined evaluation order), not `-32006` +- **T-GH-094**: A user in both `blocked-users` and `trusted-users` is denied because `blocked-users` takes precedence ### 11.2 Compliance Checklist diff --git a/scratchpad/guard-policies-specification.md b/scratchpad/guard-policies-specification.md index 8de54582417..89b282e43f1 100644 --- a/scratchpad/guard-policies-specification.md +++ b/scratchpad/guard-policies-specification.md @@ -102,6 +102,8 @@ tools: > **Note**: The field was originally named `repos` and renamed to `allowed-repos` in PR #22331. The old name is retained as a deprecated alias; run `gh aw fix` to migrate automatically. +Runtime precedence is defined by the canonical [Evaluation Order](#41-evaluation-order) in Operations. + ## Operations ### 4. MCP Gateway Configuration Flow @@ -127,6 +129,14 @@ tools: - Enforces policies on all tool invocations - Blocks unauthorized repository access +### 4.1 Evaluation Order + +The MCP Gateway MUST evaluate access in this order: + +1. `lockdown: true` takes absolute precedence and denies the invocation; `allowed-repos` and `min-integrity` MUST NOT be evaluated. +2. When lockdown is not enabled, `allowed-repos` determines whether the target repository is in scope. +3. When the repository is in scope, `min-integrity` determines whether the content meets the required integrity level. Both checks MUST pass to allow the invocation. + ### 5. Safe Outputs Integration When GitHub guard policies are configured, the compiler automatically derives a linked guard-policy for the safe-outputs MCP server. This ensures that safe output operations work correctly with guard policies by creating a write-sink configuration. @@ -403,8 +413,7 @@ tools: 3. **How should conflicts between lockdown and guard policies be resolved?** - **Decision**: `lockdown: true` takes **absolute precedence** over guard policies. When `lockdown: true` is set, all tool invocations are blocked regardless of any `allowed-repos` or `min-integrity` configuration. Guard policies are not evaluated when lockdown is active. - *Rationale*: Lockdown is an emergency/security stop; it MUST NOT be weakened by other configuration. Guard policies narrow access within an otherwise-open tool session; they do not grant access that lockdown has revoked. The compiler SHOULD warn operators at compilation time when both `lockdown: true` and guard-policy fields (`allowed-repos`, `min-integrity`, `blocked-users`, `trusted-users`, `approval-labels`) are present, as the combination is likely a misconfiguration. This warning is now implemented in `pkg/workflow/tools_validation_github.go`, where `validateGitHubGuardPolicy()` detects the conflict and `emitGitHubLockdownGuardPolicyWarning()` surfaces the compiler warning. + **Decision**: The canonical [Evaluation Order](#41-evaluation-order) applies. The compiler SHOULD warn operators at compilation time when `lockdown: true` and guard-policy fields are both present; this is implemented in `pkg/workflow/tools_validation_github.go`, where `validateGitHubGuardPolicy()` detects the conflict and `emitGitHubLockdownGuardPolicyWarning()` surfaces the warning. 4. **Should we add a "dry-run" mode to test policies before enforcement?** diff --git a/scratchpad/safe-outputs-specification.md b/scratchpad/safe-outputs-specification.md index 7bc3dfd158d..c83dcf58017 100644 --- a/scratchpad/safe-outputs-specification.md +++ b/scratchpad/safe-outputs-specification.md @@ -158,6 +158,7 @@ This subsection clarifies how the Requirements Notation (§2.2) is applied consi - When a requirement uses "SHOULD" or "RECOMMENDED", implementations that deviate MUST document the deviation and its rationale (for example in release notes or an architecture decision record). - Conflicting requirements MUST NOT appear for the same conformance class; if a later section appears to narrow an earlier "MUST", the later section is normative and the earlier section MUST be read as superseded. - Requirements scoped to a specific conformance class (§2.1) apply only to implementations claiming that class or higher; a Standard Conforming Implementation MUST also satisfy all Basic Conformance requirements. +- Changes to the Layer 3 and Layer 4 requirements MUST be reviewed using the implementation mappings in [Sync Notes](#sync-notes). --- @@ -1266,6 +1267,24 @@ The system does NOT protect against: --- +## Sync Notes + +| Specification area | Implementation mapping | +|---|---| +| §3.4 Layer 3 validation guardrails | `pkg/workflow/safe_outputs_validation.go`, `pkg/workflow/safe_outputs_validation_config.go` | +| §3.5 Layer 4 execution handlers | `pkg/workflow/safe_output_handlers.go`, `pkg/workflow/safe_outputs_actions.go` | +| §2.4 Norms | Review the Layer 3 and Layer 4 mappings above whenever a normative requirement changes. | + +## Sync Follow-ups + +When changing Layer 3 or Layer 4 behavior: + +1. Verify the mapped `pkg/workflow/` implementation and its focused tests reflect the updated requirement. +2. Update this specification's conformance requirements when the implementation changes behavior. +3. Record any intentional implementation deviation and its rationale as required by §2.4. + +--- + ## Change Log ### Version 1.1.0 (Recommendation) diff --git a/specs/awf-config-sources-compliance/README.md b/specs/awf-config-sources-compliance/README.md index 0de78c3515f..5dcb4ed8276 100644 --- a/specs/awf-config-sources-compliance/README.md +++ b/specs/awf-config-sources-compliance/README.md @@ -24,6 +24,7 @@ The following test IDs cover the `DriftRecord` schema and its usage requirements | T-DR-008 | §7.5.1 — corrective PR embeds records | The corrective PR description MUST embed the full `DriftRecord` list as JSON. | `pkg/workflow/awf_config_drift_test.go` | | T-DR-009 | §7.5.1 — empty list is valid | An empty `DriftRecord` list (no drift detected) is a valid output and MUST NOT trigger corrective PR or escalation actions. | `pkg/workflow/awf_config_drift_test.go` | | T-DR-010 | §7.2 Step 5 integration | The drift detection procedure Step 5 MUST produce a list of zero or more `DriftRecord` objects; the output format MUST be a JSON array conforming to the §3.1 schema. | `pkg/workflow/awf_config_drift_test.go` | +| T-DR-011 | §6 CR-06a — escalation-owner acknowledgement | Escalations select a non-empty owner using the documented fallback and require acknowledgement within one business day. | `pkg/workflow/awf_config_safeguards_formal_test.go` | --- @@ -55,7 +56,7 @@ Conformance tests that validate `DriftRecord` schema compliance are implemented ``` pkg/workflow/awf_config_drift_test.go — DriftRecord schema validation and usage (T-DR-001 through T-DR-010; T-DR-005: TestDriftRecord_TDR005_NoAdditionalProperties) -pkg/workflow/awf_config_safeguards_formal_test.go — unavailable-source safeguards (T-DR-SAFE-001 through T-DR-SAFE-004) +pkg/workflow/awf_config_safeguards_formal_test.go — unavailable-source safeguards (T-DR-SAFE-001 through T-DR-SAFE-004) and CR-06a escalation-owner acknowledgement (T-DR-011) ``` To run related tests: diff --git a/specs/awf-config-sources-spec.md b/specs/awf-config-sources-spec.md index 85fd5828cf0..c5570162b33 100644 --- a/specs/awf-config-sources-spec.md +++ b/specs/awf-config-sources-spec.md @@ -171,7 +171,7 @@ This subsection identifies the normative behavior that conforming AWF config sou **CR-06**: Drift categorized as "missing in gh-aw" or "spec mismatch" MUST be remediated (merged or explicitly waived with rationale) within **5 business days** of detection. For this requirement, business days are Monday-Friday in UTC, excluding weekends. If this SLA is missed, maintainers MUST open (or update) an escalation tracking issue within 1 business day. The escalation issue MUST include an owner, unblock plan, and revised ETA. -**CR-06a (Escalation Owner Assignment)**: When opening or updating an escalation tracking issue under CR-06, the assignee **SHOULD** be determined as follows: (a) the maintainer who merged the last change to the drifted property's corresponding implementation file in `pkg/workflow/` or `actions/setup/` is the **default escalation owner** (implementation guidance: this can be determined via `git log` on the relevant file, or through PR merge history); (b) if no such maintainer is identifiable (e.g., the property has never been implemented), the escalation owner **SHOULD** default to the on-call maintainer for the `github/gh-aw` repository at the time of escalation; (c) the assigned owner **MUST** be recorded in the `Owner` field of the escalation issue template and **MUST** acknowledge the assignment by commenting on the issue within 1 business day of assignment. The escalation issue **MUST NOT** be left unassigned. +**CR-06a (Escalation Owner Assignment)**: When opening or updating an escalation tracking issue under CR-06, the assignee **SHOULD** be determined as follows: (a) the maintainer who merged the last change to the drifted property's corresponding implementation file in `pkg/workflow/` or `actions/setup/` is the **default escalation owner** (implementation guidance: this can be determined via `git log` on the relevant file, or through PR merge history); (b) if no such maintainer is identifiable (e.g., the property has never been implemented), the escalation owner **SHOULD** default to the on-call maintainer for the `github/gh-aw` repository at the time of escalation; (c) the assigned owner **MUST** be recorded in the `Owner` field of the escalation issue template and **MUST** acknowledge the assignment by commenting on the issue within 1 business day of assignment. The escalation issue **MUST NOT** be left unassigned. The formal conformance fixture is [T-DR-011](awf-config-sources-compliance/README.md#driftrecord-conformance-tests), implemented in `pkg/workflow/awf_config_safeguards_formal_test.go`; production issue assignment and comment-based acknowledgement enforcement are not yet automated. --- @@ -251,7 +251,7 @@ diff -u /tmp/schema-keys.txt /tmp/ghaw-refs.txt || true A scheduled GitHub Actions workflow in `github/gh-aw` SHOULD automate this procedure. The workflow SHOULD: -- Run on a weekly schedule and on pull requests that touch AWF config handling. +- Run on a daily schedule and on pull requests that touch AWF config handling. - Fail the check (non-zero exit) when any "missing in gh-aw" drift is found. - Post a summary comment on PRs with the drift report. - Create a tracking issue when drift is detected on the scheduled run. @@ -308,7 +308,7 @@ The drift detection procedure (Section 7.2, Step 5) **MUST** produce a list of z Every invocation (scheduled, manual, or ad hoc) **MUST** attempt to refresh the canonical sources and evaluate snapshot freshness. When canonical sources in `github/gh-aw-firewall` are unavailable (GitHub outage, auth failure, transient fetch errors), agents and automation MUST apply the following safeguards: -1. The workflow **MUST** attempt to use the last-known validated local snapshot (for example cached schema/spec artifacts from the previous successful run) to keep checks deterministic. The snapshot **MUST** be stored at a stable, well-known path: `~/.cache/gh-aw/schema-consistency/last-known-snapshot/` on self-hosted runners or `/tmp/gh-aw/agent/schema-consistency/last-known-snapshot/` when the runner is ephemeral. The workflow **MUST** record the UTC refresh time when, and only when, all canonical sources refresh successfully. Snapshots older than **7 days** (168 hours from that recorded successful refresh) **MUST** be treated as expired and **MUST NOT** be used to suppress drift warnings; when a snapshot is expired, the run **MUST** be marked degraded even if the snapshot files are physically present. Implementations **SHOULD** delete snapshots older than 14 days to prevent unbounded disk use. (T-DR-SAFE-001) -2. The workflow **SHOULD** emit a warning that canonical source retrieval failed, including the failing source path(s) and timestamp. (T-DR-SAFE-002) -3. The workflow **MUST** skip destructive validation actions (for example failing required checks, auto-opening corrective PRs, or auto-creating drift issues from stale snapshots) when canonical data cannot be refreshed, and mark the run as degraded instead of silently passing. (T-DR-SAFE-003) -4. The workflow **SHOULD** open or update a tracking issue when canonical source unavailability persists from one scheduled cron invocation through the next scheduled cron invocation. Manual reruns and ad hoc invocations still perform the refresh and freshness checks above, but do not advance this consecutive-scheduled-run threshold. (T-DR-SAFE-004) +1. The workflow **MUST** attempt to use the last-known validated local snapshot (for example cached schema/spec artifacts from the previous successful run) to keep checks deterministic. The snapshot **MUST** be stored at a stable, well-known path: `~/.cache/gh-aw/schema-consistency/last-known-snapshot/` on self-hosted runners or `/tmp/gh-aw/agent/schema-consistency/last-known-snapshot/` when the runner is ephemeral. The workflow **MUST** record the UTC refresh time when, and only when, all canonical sources refresh successfully. Snapshots older than **7 days** (168 hours from that recorded successful refresh) **MUST** be treated as expired and **MUST NOT** be used to suppress drift warnings; when a snapshot is expired, the run **MUST** be marked degraded even if the snapshot files are physically present. Implementations **SHOULD** delete snapshots older than 14 days to prevent unbounded disk use. (T-DR-SAFE-001; see the [fixture index](awf-config-sources-compliance/README.md#safeguards-conformance-tests)) +2. The workflow **SHOULD** emit a warning that canonical source retrieval failed, including the failing source path(s) and timestamp. (T-DR-SAFE-002; see the [fixture index](awf-config-sources-compliance/README.md#safeguards-conformance-tests)) +3. The workflow **MUST** skip destructive validation actions (for example failing required checks, auto-opening corrective PRs, or auto-creating drift issues from stale snapshots) when canonical data cannot be refreshed, and mark the run as degraded instead of silently passing. (T-DR-SAFE-003; see the [fixture index](awf-config-sources-compliance/README.md#safeguards-conformance-tests)) +4. The workflow **SHOULD** open or update a tracking issue when canonical source unavailability persists from one daily scheduled cron invocation through the next. Manual reruns and ad hoc invocations still perform the refresh and freshness checks above, but do not advance this consecutive-scheduled-run threshold. This persistence threshold applies only to unavailable-source reporting; the daily cadence in §7.4 applies to the full drift check. (T-DR-SAFE-004; see the [fixture index](awf-config-sources-compliance/README.md#safeguards-conformance-tests)) From bca0862114f256620eda7ef831496da3efa250af Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 21 Aug 2026 16:19:10 +0000 Subject: [PATCH 3/4] Retain lockdown precedence rationale Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- scratchpad/guard-policies-specification.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/scratchpad/guard-policies-specification.md b/scratchpad/guard-policies-specification.md index 89b282e43f1..4d5674234b1 100644 --- a/scratchpad/guard-policies-specification.md +++ b/scratchpad/guard-policies-specification.md @@ -137,6 +137,8 @@ The MCP Gateway MUST evaluate access in this order: 2. When lockdown is not enabled, `allowed-repos` determines whether the target repository is in scope. 3. When the repository is in scope, `min-integrity` determines whether the content meets the required integrity level. Both checks MUST pass to allow the invocation. +Lockdown is an emergency security stop and MUST NOT be weakened by guard policies; those policies narrow access in an otherwise-open tool session but never grant access revoked by lockdown. + ### 5. Safe Outputs Integration When GitHub guard policies are configured, the compiler automatically derives a linked guard-policy for the safe-outputs MCP server. This ensures that safe output operations work correctly with guard policies by creating a write-sink configuration. From d460e6ba7879cdb4df12a43820e15d761a3a7223 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 21 Aug 2026 16:19:40 +0000 Subject: [PATCH 4/4] Apply remaining changes Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- .github/workflows/agentic_commands.yml | 1 - 1 file changed, 1 deletion(-) diff --git a/.github/workflows/agentic_commands.yml b/.github/workflows/agentic_commands.yml index 73663bc5cdf..dbb43b5ca6a 100644 --- a/.github/workflows/agentic_commands.yml +++ b/.github/workflows/agentic_commands.yml @@ -121,7 +121,6 @@ jobs: route: runs-on: ubuntu-slim timeout-minutes: 15 - # runner-guard:ignore RGS-005 -- the router requires these scoped writes to dispatch approved workflows and add their status reactions/comments; it never executes user-supplied code. permissions: actions: write contents: read