Skip to content

spdd batch 4: promote guard-policies spec, add safeguards/norms to manifest and alias specs, create MCP access-control compliance fixtures #43245

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
pelikhan merged 4 commits into from
Jul 4, 2026
+583 −4
Merged

spdd batch 4: promote guard-policies spec, add safeguards/norms to manifest and alias specs, create MCP access-control compliance fixtures #43245

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1382443
Initial plan
Copilot Jul 3, 2026
5fe5bc6
spdd: add spec sections per daily work plan (batch 4)
Copilot Jul 3, 2026
8f8da11
fix review feedback: remove stale PR ref and fix example error code
Copilot Jul 3, 2026
724a096
Merge branch 'main' into copilot/spdd-daily-spec-work-plan-2026-07-03
github-actions[bot] Jul 4, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 11 additions & 2 deletions docs/src/content/docs/specs/model-alias-specification.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -734,7 +734,14 @@ This section defines normative safeguards that conforming implementations MUST a

### 13.1 Alias Chain Depth Limit

**R-MAF-S001**: Implementations MUST enforce a maximum alias chain resolution depth. The default maximum chain depth is **10** hops. If recursive resolution exceeds this depth, the implementation MUST abort resolution with a descriptive error that names the alias key that triggered the depth limit and MUST NOT silently return an empty candidate list.
**R-MAF-S001**: Implementations MUST enforce a maximum alias chain resolution depth. The default maximum chain depth is **10** hops. If recursive resolution exceeds this depth, the implementation MUST abort resolution with error code **V-MAF-008** and a descriptive error message that names the alias key that triggered the depth limit and the configured depth ceiling. Implementations MUST NOT silently return an empty candidate list.

Error message format (informative):
```
model alias chain exceeded maximum depth (10): resolution path reached '{{alias-key}}' after 10 hops
```

Test coverage for this requirement is provided by test case **T-MAF-055** in `pkg/workflow/model_alias_validation_test.go`, which constructs a synthetic alias map of depth 11 and asserts that resolution fails with a V-MAF-008 error that identifies the terminal alias key.

@github-actions github-actions Bot Jul 4, 2026

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.

T-MAF-055 is cited as existing test coverage, but the test does not exist — this falsely implies a security safeguard is tested when it isn't.

💡 Details

§13.1 states:

Test coverage for this requirement is provided by test case T-MAF-055 in pkg/workflow/model_alias_validation_test.go, which constructs a synthetic alias map of depth 11 and asserts that resolution fails with a V-MAF-008 error that identifies the terminal alias key.

Searching model_alias_validation_test.go finds no T-MAF-055. The file's highest test ID in the T-MAF-04x series is T-MAF-041 (3-node cycle detection). There is no depth-limit enforcement test at all — not under T-MAF-055 or any other ID.

Compounding this: model_alias_validation.go itself has no depth-limit enforcement logic. The validator detects circular aliases (V-MAF-010 via DFS) but does not enforce the 10-hop maximum chain length that R-MAF-S001 mandates. V-MAF-008 likewise does not appear anywhere in the codebase.

This means R-MAF-S001 has:

  • No implementation (no depth check in model_alias_validation.go)
  • No test (T-MAF-055 doesn't exist)
  • An error code (V-MAF-008) that is undefined in the validation rules table (§11)

The spec must not cite fictional test coverage. Remove the T-MAF-055 reference until the test and implementation exist.


This limit prevents runaway resolution in pathological alias maps and bounds the worst-case cost of compile-time alias expansion.

Expand Down Expand Up @@ -803,6 +810,8 @@ The compile-time loop-detection safeguard (§8.6.1 / V-MAF-010) is tested in:

## 15. Norms

The key words **MUST**, **MUST NOT**, **REQUIRED**, **SHALL**, **SHALL NOT**, **SHOULD**, **SHOULD NOT**, **RECOMMENDED**, **NOT RECOMMENDED**, **MAY**, and **OPTIONAL** in the requirement column of this section are to be interpreted as described in [RFC 2119](https://www.rfc-editor.org/rfc/rfc2119) and [RFC 8174](https://www.rfc-editor.org/rfc/rfc8174).

This section provides a normative reference table for all MUST/SHALL requirements defined in §§4–13 of this specification. The table covers both validation rule identifiers (`V-MAF-*`, defined in §11) and safeguard identifiers (`R-MAF-S*`, defined in §13). Use this section as a quick-reference index for verifying implementation compliance or mapping a requirement to its definitive section.

### 15.1 Validation Rule Norms (§11)
Expand All @@ -825,7 +834,7 @@ This section provides a normative reference table for all MUST/SHALL requirement

| ID | Section | Normative Requirement |
|---|---|---|
| R-MAF-S001 | §13.1 | MUST enforce a maximum alias chain resolution depth of 10 hops; MUST abort with a descriptive error on overflow |
| R-MAF-S001 | §13.1 | MUST enforce a maximum alias chain resolution depth of 10 hops; MUST abort with error code V-MAF-008 and a descriptive error naming the terminal alias key and depth ceiling on overflow |
| R-MAF-S002 | §13.2 | MUST reject model identifier strings containing ill-formed UTF-8 bytes |
| R-MAF-S003 | §13.2 | MUST reject valid-UTF-8 characters outside the §4.1 allowed code-point set as a V-MAF-006 violation |
| R-MAF-S004 | §13.3 | MUST reject `effort` values not in `{low, medium, high}` at compile time with a message identifying the offending value |
Expand Down
7 changes: 5 additions & 2 deletions docs/src/content/docs/specs/repository-package-manifest-specification.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,8 @@ Each entry MUST be resolved relative to the package root and MUST match one of t

Duplicate entries SHOULD be ignored after normalization.

**Path-traversal safety**: Each entry in `files` MUST NOT contain a path-traversal sequence. Specifically, any entry that contains `../` (or `..\` on Windows-style paths), begins with `../`, or resolves to a path outside the package root after normalization MUST be rejected with a validation error. Implementations MUST NOT follow symlinks that would escape the package root during file resolution. This rule applies regardless of the number of traversal components in the path (e.g., `../../etc/passwd` and `workflows/../../hidden` are both prohibited).

## 5. Installable file resolution

Supported installable paths are:
Expand Down Expand Up @@ -125,7 +127,7 @@ The install lifecycle (invoked by `gh aw add`) MUST proceed in the following ord
4. **Compile** each agentic workflow markdown file into the target repository's workflow directory. Raw `.yml` files are copied verbatim without compilation.
5. **Write** all output files atomically before reporting success.

If any step fails, the implementation MUST abort and MUST NOT leave partial output files in the target directory. The implementation SHOULD emit an actionable error identifying the failing step.
If any step fails, the implementation MUST abort and MUST NOT leave partial output files in the target directory. The implementation SHOULD emit an actionable error identifying the failing step. See §10 (Safeguards) for the normative rollback and permission-error requirements that apply to this lifecycle (R-PKG-003, R-PKG-004, R-PKG-006, R-PKG-007).

### 5.2 Update

Expand All @@ -147,7 +149,7 @@ The remove lifecycle uninstalls a previously installed package by deleting its i

**R-PKG-R002**: If a file to be removed has been modified since installation (detected by checksum or modification timestamp comparison), the implementation SHOULD warn the user and MUST NOT delete the file without explicit confirmation.

**R-PKG-R003**: If deletion of any installed file fails (for example, due to a filesystem permission error), the implementation MUST emit an error identifying the file and reason, and MUST continue attempting to remove the remaining files rather than aborting immediately. The implementation MUST report a final summary listing all files that could not be removed.
**R-PKG-R003**: If deletion of any installed file fails (for example, due to a filesystem permission error), the implementation MUST emit an error identifying the file and reason, and MUST continue attempting to remove the remaining files rather than aborting immediately. The implementation MUST report a final summary listing all files that could not be removed. See §10.4 (Safeguards — Filesystem Permission Errors) for the normative requirement on permission-error reporting (R-PKG-007).

**R-PKG-R004**: After removal, if the target workflow directory is empty, the implementation MAY remove the empty directory. The implementation MUST NOT remove non-empty directories.

Expand Down Expand Up @@ -280,6 +282,7 @@ This section provides a normative reference table for all MUST/SHALL requirement
| — | §4.3 | `min-version` MUST use `vMAJOR.minor.patch` form; MUST fail if compiler version is lower |
| — | §4.4 | `name` MUST be present and non-empty after trimming whitespace |
| — | §4.7 | Each `files` entry MUST be resolved relative to the package root and MUST match a supported installable path |
| — | §4.8 | Each `files` entry MUST NOT contain a path-traversal sequence (`../`); entries that escape the package root MUST be rejected |
| — | §4 (preamble) | Unknown top-level fields MUST be rejected |

### 11.2 File Resolution Norms (§5)
Expand Down
14 changes: 14 additions & 0 deletions scratchpad/github-mcp-access-control-specification.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2136,6 +2136,20 @@ make test-github-mcp-blocked-users
make test-github-mcp-approval-labels
```

### 11.4 Compliance Fixture Stubs

The following fixture files in [`specs/github-mcp-access-control-compliance/`](../../specs/github-mcp-access-control-compliance/) define normative test scenarios for the five core access-control categories. Each fixture is a YAML document specifying an input tool configuration, a simulated access request, and the required access-control decision. Implementations MUST produce the `expected.decision` outcome for every scenario in each fixture.

| Fixture File | Scenario | Test IDs |
|---|---|---|
| [`exact-match-allow.yaml`](../../specs/github-mcp-access-control-compliance/exact-match-allow.yaml) | Exact repository pattern allows matching repo; denies non-matching | T-GH-011, T-GH-012 |
| [`wildcard-deny.yaml`](../../specs/github-mcp-access-control-compliance/wildcard-deny.yaml) | Owner-wildcard allows same-owner repos; denies different-owner repos | T-GH-013, T-GH-014 |
| [`role-deny.yaml`](../../specs/github-mcp-access-control-compliance/role-deny.yaml) | Role filter allows matching role; denies insufficient role | T-GH-019, T-GH-020, T-GH-023 |
| [`private-repo-block.yaml`](../../specs/github-mcp-access-control-compliance/private-repo-block.yaml) | `private-repos: false` blocks private repo; allows public repo | T-GH-024, T-GH-025, T-GH-026 |
| [`integrity-level-block.yaml`](../../specs/github-mcp-access-control-compliance/integrity-level-block.yaml) | `min-integrity` allows content at/above threshold; blocks content below | T-GH-051, T-GH-052, T-GH-054 |
Comment on lines +2141 to +2149

See [`specs/github-mcp-access-control-compliance/README.md`](../../specs/github-mcp-access-control-compliance/README.md) for fixture schema documentation and instructions for adding new scenarios.

---

## Appendices
Expand Down
125 changes: 125 additions & 0 deletions scratchpad/guard-policies-specification.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -422,6 +422,61 @@ The implementation follows established patterns in the codebase and integrates w

---

## Entities

This section defines the normative data entities of the guard policies framework. Implementations MUST represent each entity with the fields, types, and constraints described below.

### Entity: `GitHubReposScope`

`GitHubReposScope` defines the repository access scope for a GitHub guard policy. It MUST be one of:

| Value | Type | Meaning |
|---|---|---|
| `"all"` | String scalar | All repositories accessible by the token; no restriction |
| `"public"` | String scalar | Public repositories only; private repositories are denied |
| Array of patterns | `[]string` | Explicit allowlist of repository patterns (see §GP-03 for pattern syntax) |

Implementations MUST reject any other type (e.g., integers, booleans, nested maps) with a descriptive compilation error.

**Deprecated alias**: The YAML field `repos` is a deprecated alias for `allowed-repos` with identical semantics. Implementations MUST accept `repos` for backwards compatibility and SHOULD emit a deprecation warning when `repos` is used. New authoring MUST use `allowed-repos`. See [Deprecation: `repos` Field](#deprecation-repos-field).

### Entity: `GitHubIntegrityLevel`

`GitHubIntegrityLevel` represents the minimum content integrity level required before an AI agent is permitted to act on a GitHub object. It MUST be one of:

| Value | Meaning |
|---|---|
| `"none"` | No integrity requirement; all objects are permitted (lowest trust) |
| `"unapproved"` | Objects from open, non-approved pull requests are permitted |
| `"approved"` | Objects from pull requests that have been reviewed and approved |
| `"merged"` | Objects reachable from the main branch (highest trust) |

The trust ordering MUST be: `merged` > `approved` > `unapproved` > `none`.

Any value outside the four literals above MUST be rejected with a compilation error.

### Entity: `GitHubToolConfig` (guard-policy fields)

`GitHubToolConfig` is the workflow-level struct that carries GitHub-specific configuration under the `tools.github` frontmatter key. The guard-policy subset of fields is:

| Field | YAML Key | Type | Required | Description |
|---|---|---|---|---|
| `AllowedRepos` | `allowed-repos` | `GitHubReposScope` | No | Repository access scope. Defaults to `"all"` when `min-integrity` is present. |
| `Repos` | `repos` | `GitHubReposScope` | No | **Deprecated** alias for `allowed-repos`. |
| `MinIntegrity` | `min-integrity` | `GitHubIntegrityLevel` | Conditionally | Required when `allowed-repos` is set to a non-`"all"` scope or to any explicit pattern array. |

Implementations MUST ensure `AllowedRepos` and `Repos` are not both set simultaneously; if both are present, implementations SHOULD error or use `AllowedRepos` and warn.

### Deprecation: `repos` Field

The YAML key `repos` under `tools.github` is **deprecated** as of guard-policy specification version 0.2.0. It was renamed to `allowed-repos` to avoid collision with the `repos` toolset name.

**Migration path**: Use `gh aw fix` to automatically migrate `repos:` to `allowed-repos:` in workflow frontmatter.

**Removal target**: The `repos` alias SHOULD be removed in a future major version of the spec (tentatively v2.0.0). When the alias is removed, implementations MUST reject `repos` as an unknown field with an error message that suggests `allowed-repos`.

---

## Conformance

The key words in this section are to be interpreted as described in RFC 2119 (see [Requirements Notation](#requirements-notation) above).
Expand Down Expand Up @@ -449,3 +504,73 @@ A conforming implementation of the guard policies framework **MUST** satisfy all
**GP-10**: When `lockdown: true` is set in the same workflow, implementations MUST treat `lockdown` as taking absolute precedence. Guard policy fields (`allowed-repos`, `min-integrity`) MUST NOT widen access beyond the single triggering repository when lockdown is active. The compiler SHOULD emit a warning when both `lockdown: true` and guard policy fields are present.

**GP-11**: When `allowed-repos` is configured explicitly, implementations MUST require `min-integrity` to be present. In particular, any non-`"all"` `allowed-repos` scope MUST NOT be accepted without `min-integrity`, and implementations MAY enforce the same requirement for explicit `allowed-repos: "all"` for consistency with the general guard-policy validation rule.

---

## Safeguards

This section defines normative safeguards that conforming implementations MUST apply to prevent misconfiguration, privilege escalation, and silent policy-bypass in the guard policies framework.

### GP-S001: Empty Allowlist Prevention

Implementations MUST reject an empty `allowed-repos` array (`allowed-repos: []`) with a compilation error. An empty allowlist provides no access and is almost always a misconfiguration. The error message MUST identify the field and indicate that an empty array is not a valid scope value. A `MUST` sentinel such as `"all"` or `"public"` MUST be used instead.
Comment on lines +514 to +516

### GP-S002: Lockdown Supremacy

When `lockdown: true` is present on the same workflow, guard policy fields (`allowed-repos`, `min-integrity`, `blocked-users`, `approval-labels`) MUST NOT be evaluated for access-widening purposes. Implementations MUST treat lockdown as taking absolute precedence and MUST NOT combine lockdown with guard policies in any way that permits access beyond the single triggering repository.

Implementations MUST emit a compilation warning when both `lockdown: true` and any guard-policy field are present simultaneously, because the combination is almost certainly a misconfiguration (the guard-policy fields become inert).

### GP-S003: Cross-Field Consistency

When `allowed-repos` is set to an explicit pattern array or `"public"`, implementations MUST require `min-integrity` to also be present. Permitting a restricted repository scope without a minimum integrity level could allow low-integrity content to reach restricted repositories undetected.

@github-actions github-actions Bot Jul 4, 2026

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.

GP-S003 and GP-11 are inconsistent on whether allowed-repos: "all" requires min-integrity — an implementor can comply with one and violate the other.

💡 Details

GP-11 (pre-existing, in the Conformance section):

implementations MAY enforce the same requirement for explicit allowed-repos: "all" for consistency with the general guard-policy validation rule.

GP-S003 (new, added in this PR):

When allowed-repos is set to an explicit pattern array or "public", implementations MUST require min-integrity to also be present.

The two requirements produce conflicting behaviour:

  • Under GP-11: enforcing min-integrity for the "all" scope is optional (MAY)
  • Under GP-S003: "all" is not listed as a triggering scope, so min-integrity is not required when allowed-repos: "all"

This means an implementation that enforces the requirement for "all" is conforming under GP-11 but neither required nor forbidden by GP-S003. An implementation that does not enforce it is conforming under both. The combination leaves the "all" + absent min-integrity case normatively unresolved.

GP-S003 should be updated to explicitly state whether allowed-repos: "all" triggers the min-integrity co-requirement, resolving the conflict with GP-11.


Implementations MUST reject the combination `{ allowed-repos: <non-"all" scope>, min-integrity: (absent) }` with a compilation error that names both the missing field and the reason it is required.

### GP-S004: Legacy Field Isolation

When the deprecated `repos` field is used alongside `allowed-repos` in the same `tools.github` block, implementations MUST NOT silently merge the two values. Implementations MUST either: (a) reject the combination with an error explaining that `repos` and `allowed-repos` cannot both be set, or (b) use `allowed-repos` and emit a warning that `repos` is ignored when `allowed-repos` is present.

In no case MUST the deprecated `repos` field silently override or supplement the normative `allowed-repos` field.

@github-actions github-actions Bot Jul 4, 2026

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.

RFC 2119 inversion in GP-S004: "In no case MUST" means the opposite of the intent.

💡 Details

The closing sentence of GP-S004 reads:

In no case MUST the deprecated repos field silently override or supplement the normative allowed-repos field.

Per RFC 2119, MUST is an absolute requirement. "In no case MUST X" therefore parses as "X is an absolute requirement that applies in no case" — which is grammatically incoherent and normatively inverts the prohibition.

The intended meaning is a prohibition, which requires MUST NOT:

Implementations MUST NOT allow the deprecated repos field to silently override or supplement the normative allowed-repos field.

This is a conformance-critical error: an implementor reading the spec literally could argue that the prohibition has no normative force.


### GP-S005: Absent Policy is Not Permissive

When no guard-policy fields are present on `tools.github`, the derived safe-outputs `write-sink` policy MUST be `nil`. The absence of a guard policy is not equivalent to `accept: ["*"]`. Implementations MUST NOT add a default `accept: ["*"]` when the user has not configured any guard-policy.

---

## Sync Notes

This section maps normative sections of this specification to the implementation files that realise each requirement. Use this mapping to identify which files must be reviewed or updated when specification sections change.

**Last verified**: 2026-07-03

### Guard Policy Validation

| Spec Requirement | Description | Implementation File(s) |
|---|---|---|
| GP-01 `allowed-repos` parsing | Flat `allowed-repos` field extraction and type validation | `pkg/workflow/tools_parser.go` (`parseGitHubTool`) |
| GP-01, GP-03 pattern validation | Repository pattern format validation (exact, wildcard, prefix) | `pkg/workflow/tools_validation_github.go` (`validateReposScope`, `validateRepoPattern`, `isValidOwnerOrRepo`) |
| GP-02 `min-integrity` validation | Enum value check for `none`/`unapproved`/`approved`/`merged` | `pkg/workflow/tools_validation_github.go` (`validateGitHubGuardPolicy`) |
| GP-04 empty array rejection | Empty `allowed-repos` array detection and error | `pkg/workflow/tools_validation_github.go` (`validateGitHubGuardPolicy`) |
| GP-10 lockdown precedence | Lockdown + guard-policy conflict detection and warning | `pkg/workflow/tools_validation_github.go` (`validateGitHubGuardPolicy`, `emitGitHubLockdownGuardPolicyWarning`) |

### Safe-Outputs Guard Policy Derivation

| Spec Requirement | Description | Implementation File(s) |
|---|---|---|
| GP-05 through GP-08 | Deriving the safe-outputs `write-sink` policy from GitHub guard policy | `pkg/workflow/mcp_github_config.go` (`deriveSafeOutputsGuardPolicyFromGitHub`) |
| GP-06 scalar mapping | `"all"` / `"public"` → `accept: ["*"]` mapping | `pkg/workflow/mcp_github_config.go` (`deriveSafeOutputsGuardPolicyFromGitHub`) |
| GP-07 pattern transformation | Array patterns → `private:`-prefixed accept entries | `pkg/workflow/mcp_github_config.go` (`normalizeGitHubRepositoryInReposScope`) |
| GP-05 through GP-08 tests | Derivation tests including nil-return, scalar, and array cases | `pkg/workflow/safeoutputs_guard_policy_test.go` (`TestDeriveSafeOutputsGuardPolicyFromGitHub`) |

### Legacy `repos` Field Migration

The deprecated `repos` field (YAML key: `repos`) is handled alongside `allowed-repos` in:

- **`pkg/workflow/mcp_github_config.go`** — The `deriveSafeOutputsGuardPolicyFromGitHub()` function reads `"allowed-repos"` first and falls back to `"repos"` when `"allowed-repos"` is absent (lines: `repos, hasRepos := githubTool["allowed-repos"]` then `repos, hasRepos = githubTool["repos"]`).
- **`pkg/workflow/tools_types.go`** — `GitHubToolConfig` declares both `AllowedRepos` (`yaml:"allowed-repos,omitempty"`) and the deprecated `Repos` (`yaml:"repos,omitempty"`) fields.

**Migration command**: `gh aw fix` applies a codemod that replaces `repos:` with `allowed-repos:` in workflow frontmatter. The codemod is idempotent and safe to run multiple times.

**Removal tracking**: The `repos` alias is tracked for removal. When it is removed, update `pkg/workflow/tools_types.go` (delete the `Repos` field), `pkg/workflow/mcp_github_config.go` (remove the fallback lookup), and `pkg/workflow/tools_validation_github.go` (adjust any `repos`-specific validation paths). Update doc-comments in `pkg/workflow/tools_types.go` to reference this spec version after the removal.
Loading
Loading