-
Notifications
You must be signed in to change notification settings - Fork 454
SPDD 2026-06-30: tighten normative requirements across 5 specs #42530
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.
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -230,6 +230,18 @@ Catalog refresh processes SHOULD use normalized upstream model inventories and S | |
|
|
||
| Catalog updates MUST preserve JSON validity and MUST maintain backward-safe handling for historical model IDs that remain in the catalog but are absent from current live inventories. | ||
|
|
||
| ### 5.4 Catalog Sync Failure Handling | ||
|
|
||
| A conforming implementation MUST define and enforce behavior for catalog synchronization failures. Specifically: | ||
|
|
||
| 1. **Unreachable catalog source**: When the upstream pricing catalog source is unreachable during a sync operation, the implementation MUST fall back to the last successfully synchronized catalog version. The implementation MUST NOT silently use an empty or zero-cost catalog. The implementation MUST emit a diagnostic warning identifying the sync failure and the fallback version in use. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. §5.4 item 1 is silent on the first-run case where no prior sync exists: The fallback to 💡 Suggested fixAdd a normative clause for the bootstrap case, for example:
Without this, every implementation independently invents its own bootstrap behavior, which is exactly the kind of undefined-corner that leads to divergent security properties. |
||
|
|
||
| 2. **Malformed catalog**: When a fetched catalog update fails JSON schema validation or contains non-numeric cost fields, the implementation MUST reject the update entirely, retain the previous valid catalog, and emit a validation error identifying the malformed field(s). The implementation MUST NOT partially apply a malformed update. | ||
|
|
||
| 3. **Missing required fields**: When a catalog entry is missing required fields (`input` or `output` cost values), the implementation MUST treat that entry as invalid and MUST NOT compute AIC using zero or undefined costs for that model. The entry MUST be skipped or flagged, and a diagnostic MUST be emitted. | ||
|
|
||
| 4. **Catalog version mismatch**: When the two required catalog paths (`pkg/cli/data/models.json` and `actions/setup/js/models.json`) diverge in content after a sync operation, the sync tooling/CI gate MUST treat this as a sync failure and MUST fail the refresh operation until consistency is restored. | ||
|
|
||
| --- | ||
|
|
||
| ## 6. Copilot Billing Reference Requirements | ||
|
|
@@ -495,11 +507,12 @@ Conforming implementations SHOULD surface explicit validation errors for: | |
|
|
||
| ### Appendix C: Security and Integrity Considerations | ||
|
|
||
| Pricing catalogs are configuration inputs. Implementations SHOULD: | ||
| Pricing catalogs are configuration inputs. Implementations MUST: | ||
|
|
||
| - Treat catalog updates as controlled changes. | ||
| - Validate and review catalog source provenance. | ||
| - Treat catalog updates as controlled changes and gate all updates through the project's standard change-control workflow. | ||
| - Validate catalog source provenance before applying any update; the source origin MUST be confirmed as an authorized upstream before the catalog is consumed. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. "Authorized upstream" is undefined — normative MUST references an undefined term. "The source origin MUST be confirmed as an authorized upstream before the catalog is consumed" — the term "authorized upstream" is not defined anywhere in this specification. A normative MUST requirement that relies on an undefined term creates an unverifiable conformance requirement. Define "authorized upstream" in the Glossary (or inline here), specifying at minimum: how authorization is established, where the list of authorized upstreams is maintained, and what mechanism (e.g., URL allowlist, signing key, CODEOWNERS entry) is used to confirm it. @copilot please address this.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
💡 Suggested fixDefine the mechanism. For example:
Without a mechanism, this MUST cannot be verified by an auditor, and cannot be implemented consistently across different teams consuming this spec. |
||
| - Avoid silently mutating cost values at runtime. | ||
| - Review catalog changes for correctness and billing alignment before merging; reviews MUST be performed by at least one team member with knowledge of the active model pricing. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Subjective process requirement embedded in a normative technical spec. "Reviews MUST be performed by at least one team member with knowledge of the active model pricing" — "knowledge of the active model pricing" is a subjective, unverifiable qualification. This introduces a MUST requirement that cannot be tested for conformance in any automated or objective way. Process and qualification requirements of this kind belong in a CONTRIBUTING.md or code-review process document, not in a normative technical specification. If this must remain in the spec, the qualification criterion needs to be made objective and verifiable (e.g., referencing a specific role or CODEOWNERS group). @copilot please address this.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Elevating to MUST a review requirement defined only by subjective expertise makes it unverifiable and unenforceably broad: 💡 Suggested fixEither define a role with formal qualification (e.g.,
A MUST obligation that cannot be objectively verified is normative debt — it degrades trust in all MUST requirements in the spec. |
||
|
|
||
| --- | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -332,6 +332,33 @@ When a permission request is denied, the implementation MUST: | |||||||||
|
|
||||||||||
| In standalone mode, missing required environment variables or unreadable prompt input MUST be reported to standard error with a driver-specific prefix. | ||||||||||
|
|
||||||||||
| ### 6.5 Structured Event Serialization Schema | ||||||||||
|
|
||||||||||
| All structured log entries emitted by a conforming implementation MUST conform to the following field schema. Implementations MUST include all required fields and MAY include additional fields. | ||||||||||
|
|
||||||||||
| #### 6.5.1 Lifecycle Event Fields | ||||||||||
|
|
||||||||||
| | Field | Type | Required | Description | | ||||||||||
| |-------|------|----------|-------------| | ||||||||||
| | `event` | string | MUST | Event type identifier (e.g., `"connection.start"`, `"session.created"`, `"prompt.dispatched"`, `"session.completed"`, `"error"`) | | ||||||||||
| | `timestamp` | string (ISO 8601) | MUST | UTC timestamp of the event | | ||||||||||
| | `sessionId` | string | MUST when available | Session identifier assigned at session creation; MUST be omitted before session is established | | ||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. "MUST when available" is not a standard RFC 2119 keyword and creates conformance ambiguity. The Required column says "MUST when available" for The description already captures the conditional correctly: "MUST be omitted before session is established." Suggest replacing the Required column value with This makes the requirement unambiguously testable in both the pre- and post-session states. @copilot please address this.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
💡 Suggested fixSplit into two clearly-scoped rows, or use unambiguous RFC 2119 language:
Alternatively, define a separate schema for pre-session log entries rather than embedding a conditional MUST in a shared table. |
||||||||||
| | `durationMs` | number | MUST for completion events | Elapsed time in milliseconds from prompt dispatch to completion | | ||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
💡 Suggested fixAdd a definition, either inline or in a preceding note — e.g.:\n\n> Completion event: A lifecycle log entry with Without this, the MUST obligation for |
||||||||||
| | `hasOutput` | boolean | MUST for completion events | Indicates whether the session produced a non-empty output | | ||||||||||
| | `level` | string | MUST | Log severity level: one of `"info"`, `"warning"`, `"error"` | | ||||||||||
| | `message` | string | MUST | Human-readable description of the event | | ||||||||||
|
|
||||||||||
| #### 6.5.2 Policy-Denial Log Entry Fields | ||||||||||
|
|
||||||||||
| | Field | Type | Required | Description | | ||||||||||
| |-------|------|----------|-------------| | ||||||||||
| | `event` | string | MUST | MUST be `"permission.denied"` | | ||||||||||
| | `timestamp` | string (ISO 8601) | MUST | UTC timestamp of the denial | | ||||||||||
| | `sessionId` | string | MUST | Session identifier in which the denial occurred | | ||||||||||
| | `requestKind` | string | MUST | Permission request kind: one of `"read"`, `"write"`, `"url"`, `"shell"`, `"mcp"`, `"custom-tool"` | | ||||||||||
| | `requestSummary` | string | MUST | Compact human-readable summary of the denied request (MUST NOT include secret values or raw token content) | | ||||||||||
| | `level` | string | MUST | MUST be `"warning"` or `"error"` | | ||||||||||
|
|
||||||||||
| --- | ||||||||||
|
|
||||||||||
| ## 7. Error Handling and Exit Behavior | ||||||||||
|
|
@@ -436,12 +463,18 @@ Implementations MUST provide automated tests for all Level 1 and Level 2 require | |||||||||
|
|
||||||||||
| ### Appendix C: Security Considerations | ||||||||||
|
|
||||||||||
| #### Glossary | ||||||||||
|
|
||||||||||
| **Non-ephemeral events**: Events that persist beyond the lifetime of a single session or request and are written to durable storage (for example, audit logs, telemetry sinks, or artifact uploads). Ephemeral events, by contrast, exist only in process memory or transient I/O buffers and are discarded when the driver process exits. | ||||||||||
|
|
||||||||||
| #### Requirements | ||||||||||
|
|
||||||||||
| A conforming implementation SHOULD: | ||||||||||
|
|
||||||||||
| - Treat connection tokens as secrets and avoid logging raw token values. | ||||||||||
| - Apply least-privilege permission rules and avoid broad allow-all configurations unless operationally justified. | ||||||||||
| - Preserve auditable denial logs for policy and incident review. | ||||||||||
| - Restrict event persistence to non-ephemeral events and avoid writing sensitive transient state. | ||||||||||
| - Restrict event persistence to non-ephemeral events (as defined above) and avoid writing sensitive transient state to durable storage. | ||||||||||
| - Not attempt to read, fall back to, or check for platform authentication tokens (`GITHUB_TOKEN`, `COPILOT_GITHUB_TOKEN`, `GH_TOKEN`). These tokens are not present in the driver subprocess environment by design. Attempting to use them would fail silently or cause unexpected behavior. | ||||||||||
|
|
||||||||||
| --- | ||||||||||
|
|
||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -472,16 +472,81 @@ concurrency: | |
|
|
||
| --- | ||
|
|
||
| ## Revalidation Cadence | ||
| ## Re-validation Triggers | ||
|
|
||
| Whenever any of the following occur, re-run this validation: | ||
| ### Normative Triggers | ||
|
|
||
| A conforming maintainer MUST re-run this validation when any of the following occur: | ||
|
|
||
| 1. `specs/security-architecture-spec.md` changes any MUST/SHALL-level requirement, compliance-test mapping, or security guarantee wording. | ||
| 2. Compiler or runtime changes alter compiled job structure, permission separation, threat-detection placement, timestamp validation, or other `.lock.yml` security behaviors described in this report. | ||
| 3. Companion security specifications (`scratchpad/guard-policies-specification.md`, `scratchpad/github-mcp-access-control-specification.md`) add or revise GitHub guard-policy or runtime-access-control behaviors that the top-level security architecture depends on. | ||
| 4. Validation evidence files, example workflows, or implementation locations cited in the Detailed Validation table move or change substantially. | ||
|
|
||
| For each revalidation pass, reviewers SHOULD rerun the Detailed Validation procedure above, refresh the evidence-location table, update the Minor Discrepancies section, and revise the validation grade if any claim is no longer fully verified. | ||
| For each re-validation pass, reviewers MUST rerun the Detailed Validation procedure above, refresh the evidence-location table, update the Minor Discrepancies section, and revise the validation grade if any claim is no longer fully verified. | ||
|
|
||
| ### Failure Escalation | ||
|
|
||
| When a re-validation pass identifies a specification claim that is no longer verifiable against the implementation: | ||
|
|
||
| 1. The validation grade MUST be downgraded to reflect the unverified claim (e.g., from **A** to **B** or lower). | ||
| 2. A tracking issue MUST be opened in the repository describing the gap between specification claim and implementation evidence, referencing the affected requirement identifier (e.g., `OI-01`, `IS-04`). | ||
| 3. The affected row in the Specification Accuracy Summary table MUST be updated to ❌ **UNVERIFIED** with a link to the tracking issue. | ||
| 4. No new security-impacting features that depend on the unverified claim MUST be merged until the claim is re-verified or the specification is amended to match the implementation. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Grammatically incorrect normative statement — creates opposite meaning. "No new security-impacting features that depend on the unverified claim MUST be merged" — the double negative ("No ... MUST be merged") is grammatically incorrect and produces the opposite of the intended meaning. As written, it could be parsed as: features MUST be merged, which contradicts the intent. The intended normative statement is: "Security-impacting features that depend on the unverified claim MUST NOT be merged until the claim is re-verified or the specification is amended to match the implementation." This is a correctness issue in a normative MUST statement and must be fixed before merge. @copilot please address this.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Normative inversion makes this requirement the opposite of its intent: 💡 Suggested fixChange from: To: As written, a compliant maintainer reading this normatively is required to merge dependent features — the exact opposite of the safety gate this step is meant to enforce. |
||
| 5. A re-validation MUST be performed and the validation grade MUST be restored before the tracking issue is closed. | ||
|
|
||
| --- | ||
|
|
||
| ## Automation Approach | ||
|
|
||
| ### Proposed CI Mechanism | ||
|
|
||
| To reduce manual re-validation burden and catch specification drift early, the following CI mechanism is proposed for adoption: | ||
|
|
||
| **Approach 1 — Compiled YAML structural assertions (recommended)**: Add a CI job that runs after `make recompile` and asserts structural properties of compiled `.lock.yml` files against the security architecture requirements documented in this report. Assertions MUST include: | ||
|
|
||
| - Presence and ordering of `pre_activation`, `activation`, `agent`, `detection`, and `safe_outputs` jobs. | ||
| - Permission set on `agent` job is read-only (`contents: read`, no write permissions). | ||
| - Permission set on `safe_outputs` job contains at least one write permission. | ||
| - All non-local `uses:` action references (excluding local paths such as `./actions/...`) match the SHA-pinned format (`owner/action@<40-hex-chars>`). | ||
| - `concurrency.group` contains at least one dynamic expression (`${{ ... }}`). | ||
|
|
||
| This job SHOULD be implemented as a lightweight script (e.g., a YAML-parsing shell or Node.js script) that exits non-zero when any assertion fails, blocking merge. | ||
|
|
||
| **Approach 2 — Spec-to-implementation cross-reference linter**: Extend the existing `gh aw compile` linter to emit warnings when compiled output omits patterns required by the security architecture specification (e.g., missing timestamp validation step in activation job, missing fork-protection condition in `if:` expressions). | ||
|
|
||
| A conforming CI pipeline SHOULD implement at least Approach 1 and SHOULD run the assertion suite on every pull request that modifies `.github/workflows/*.md` files or `specs/security-architecture-spec.md`. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. SHOULD for the CI approach but MUST for assertions within it — internal normative inconsistency. Line 506 uses "Assertions MUST include" (absolute requirement) for the contents of the CI job, while line 518 says "A conforming CI pipeline SHOULD implement at least Approach 1" (optional). This creates an internal inconsistency: if adoption of Approach 1 is optional (SHOULD), then the MUST constraints within it are unreachable for pipelines that choose not to adopt it. Options:
@copilot please address this.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The 💡 Suggested fixThe source workflow authoring files are Markdown under |
||
|
|
||
| --- | ||
|
|
||
| ## §12 Compliance Test Matrix Gap Analysis | ||
|
|
||
| This section audits the compliance test matrix defined in `security-architecture-spec.md §12` against the evidence documented in this validation report. Each test category is classified as **EVIDENCED**, **PARTIALLY EVIDENCED**, or **GAP** based on whether this document provides concrete implementation evidence. | ||
|
|
||
| | Test Category | Test IDs | Status | Notes | | ||
| |---------------|----------|--------|-------| | ||
| | Input Sanitization | T-IS-001 to T-IS-008 | ✅ EVIDENCED | Covered in §1a (IS-04 to IS-09); all sanitization functions verified in `sanitize_content_core.cjs` | | ||
| | Output Isolation | T-OI-001 to T-OI-007 | ⚠️ PARTIALLY EVIDENCED | OI-01 (job architecture) and OI-06 (output validation) verified; OI-02 (agent read-only) implicitly verified via permission blocks; T-OI-003 (safe output type support), T-OI-004 (output validation rules), T-OI-005 (token precedence), T-OI-006 (token secret expression), T-OI-007 (write operation isolation) lack dedicated evidence entries | | ||
| | Network Isolation | T-NI-001 to T-NI-009 | ⚠️ PARTIALLY EVIDENCED | AWF binary installation (T-NI-007) verified; T-NI-001 (network mode support), T-NI-002 (ecosystem expansion), T-NI-003 (domain matching), T-NI-004 (protocol filtering), T-NI-005 (invalid protocol), T-NI-006 (blocked domain precedence), T-NI-008 (MCP isolation), T-NI-009 (content sanitization integration) lack dedicated evidence entries | | ||
| | Permission Management | T-PM-001 to T-PM-007 | ⚠️ PARTIALLY EVIDENCED | PM-01/PM-02 (permission defaults), PM-08 (fork protection), PM-10/PM-11 (RBAC) verified; T-PM-003 (strict mode), T-PM-005 (repository validation for `workflow_run`), T-PM-007 (token validation) lack dedicated evidence entries | | ||
| | Sandbox Isolation | T-SI-001 to T-SI-007 | ❌ GAP | No sandbox isolation evidence entries are present in this validation document; T-SI-001 through T-SI-007 (AWF chroot, Docker socket, `--env-all`, GOROOT, MCP container, network isolation independence) are not covered | | ||
| | Threat Detection | T-TD-001 to T-TD-007 | ⚠️ PARTIALLY EVIDENCED | TD-01 (automatic threat detection) verified via `detection:` job; T-TD-002 (prompt injection), T-TD-003 (secret leaks), T-TD-004 (malicious patches), T-TD-005 (custom prompt), T-TD-006 (engine override), T-TD-007 (workflow failure on detection) lack dedicated evidence entries | | ||
| | Compilation-Time Security | T-CS-001 to T-CS-006, T-SG07-001, T-SG07-002 | ⚠️ PARTIALLY EVIDENCED | CS-10 (action pinning, T-CS-005) verified; T-CS-001 (schema validation), T-CS-002 (expression safety), T-CS-003 (permission validation), T-CS-004 (network config validation), T-CS-006 (deprecated feature rejection), T-SG07-001 and T-SG07-002 (fail-secure behaviors) lack dedicated evidence entries | | ||
| | Runtime Security | T-RS-001 to T-RS-011 | ⚠️ PARTIALLY EVIDENCED | RS-01/RS-02 (timestamp validation) and RS-16 to RS-22 (concurrency control) verified; T-RS-003 through T-RS-008 (repository validation for `workflow_run`, role validation, token validation, AWF/MCP network enforcement, output validation) lack dedicated evidence entries | | ||
| | Companion MCP Access-Control | T-GH-047 to T-GH-060 | ⚠️ PARTIALLY EVIDENCED | Deferred to companion specifications (`scratchpad/github-mcp-access-control-specification.md`, `scratchpad/guard-policies-specification.md`); not directly evidenced in this document | | ||
|
|
||
| ### Gap Summary | ||
|
|
||
| The following test categories require dedicated evidence entries to achieve full coverage in this validation document: | ||
|
|
||
| 1. **Sandbox Isolation (T-SI-001 to T-SI-007)** — No evidence present. A re-validation pass MUST document how AWF chroot behavior, Docker socket visibility, and MCP container isolation are reflected in compiled workflow behavior. | ||
| 2. **Output Isolation (T-OI-003 to T-OI-007)** — Partial evidence. Missing evidence for token precedence, secret expression validation, and write operation isolation. | ||
| 3. **Network Isolation (T-NI-001 to T-NI-009)** — Partial evidence. Network mode, domain matching, protocol filtering, and MCP isolation behavior are unverified in this document. | ||
| 4. **Threat Detection (T-TD-002 to T-TD-007)** — Partial evidence. Only TD-01 (job presence) verified; detection capability assertions unverified. | ||
| 5. **Compilation-Time Security (T-CS-001 to T-CS-004, T-CS-006, T-SG07)** — Partial evidence. Only action pinning (T-CS-005) verified. | ||
| 6. **Runtime Security (T-RS-003 to T-RS-008)** — Partial evidence. Only timestamp and concurrency verified. | ||
|
|
||
| Maintainers SHOULD address gaps in order of risk: Sandbox Isolation and Network Isolation are the highest-priority gaps given their role in containing agent execution. | ||
|
|
||
| --- | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing first-run edge case in MUST requirement.
The requirement says "the implementation MUST fall back to the last successfully synchronized catalog version", but does not define behavior when no prior version exists (e.g., on first run or after a clean installation). If there is no prior synchronized catalog, this MUST requirement cannot be satisfied and the implementation has no normative guidance on how to proceed safely.
Suggest adding a clause such as: "If no prior synchronized catalog exists, the implementation MUST fail closed and MUST NOT operate until a valid catalog can be obtained."
@copilot please address this.