Close SPDD daily spec-review action items for security-architecture and access-control specs - #52531
Merged
Merged
Conversation
10 tasks
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…trol spec Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…koff example Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Copilot
AI
changed the title
[WIP] Review and validate 5 specification files for SPDD
Close SPDD daily spec-review action items for security-architecture and access-control specs
Aug 13, 2026
Contributor
|
Thanks for closing out the daily SPDD review items! 🎯 This PR comprehensively addresses spec validation gaps across five interconnected specification files: ✅ What's included:
✅ Process adherence: Note: This PR is flagged 🟡 only because specs lack traditional unit test coverage. That's expected and appropriate for documentation updates. The spec changes are focused, well-described, and cross-referenced per the daily SPDD work plan. Ready for merge once reviewers sign off! 🚀
|
Contributor
There was a problem hiding this comment.
Pull request overview
Updates security and safe-output specifications to close SPDD review gaps and align documentation with current implementations.
Changes:
- Adds compiled job-structure and enforcement examples.
- Maps safe-output sections to implementation files.
- Clarifies access-control predicates and rate-limit handling.
Show a summary per file
| File | Description |
|---|---|
specs/security-architecture-spec.md |
Adds job-wiring example. |
specs/security-architecture-spec-validation.md |
Closes discrepancies and records alias decision. |
specs/security-architecture-spec-summary.md |
Classifies safeguard enforcement timing. |
specs/safe-output-outcome-evaluation.md |
Maps output types to source files. |
scratchpad/github-mcp-access-control-specification.md |
Clarifies predicates and retry behavior. |
Review details
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 5/5 changed files
- Comments generated: 6
- Review effort level: Balanced
| | Practice | Enforcement | Mechanism | | ||
| |---|---|---| | ||
| | BP-01 Sanitized context | Compile-time | Compiler rewrites `${{ github.event.* }}` expressions in `prompt:` to `${{ steps.sanitized.outputs.text }}` during compilation; unsanitized expressions never reach the generated `.lock.yml` | | ||
| | BP-02 Strict mode for production | Compile-time | `strict: true` causes the compiler to reject workflows with write permissions on the `agent` job or missing `safe-outputs:`; violations fail `gh aw compile` | |
| |---|---|---| | ||
| | BP-01 Sanitized context | Compile-time | Compiler rewrites `${{ github.event.* }}` expressions in `prompt:` to `${{ steps.sanitized.outputs.text }}` during compilation; unsanitized expressions never reach the generated `.lock.yml` | | ||
| | BP-02 Strict mode for production | Compile-time | `strict: true` causes the compiler to reject workflows with write permissions on the `agent` job or missing `safe-outputs:`; violations fail `gh aw compile` | | ||
| | BP-03 Specific domain allowlists | Runtime | The AWF network proxy/firewall enforces the configured `network.allowed` domain list against outbound requests during workflow execution; a wildcard (`"*"`) is accepted at compile time but only its effect is observed at runtime | |
|
|
||
| | Practice | Enforcement | Mechanism | | ||
| |---|---|---| | ||
| | BP-01 Sanitized context | Compile-time | Compiler rewrites `${{ github.event.* }}` expressions in `prompt:` to `${{ steps.sanitized.outputs.text }}` during compilation; unsanitized expressions never reach the generated `.lock.yml` | |
| | BP-02 Strict mode for production | Compile-time | `strict: true` causes the compiler to reject workflows with write permissions on the `agent` job or missing `safe-outputs:`; violations fail `gh aw compile` | | ||
| | BP-03 Specific domain allowlists | Runtime | The AWF network proxy/firewall enforces the configured `network.allowed` domain list against outbound requests during workflow execution; a wildcard (`"*"`) is accepted at compile time but only its effect is observed at runtime | | ||
| | BP-04 Pin actions to SHAs | Compile-time (advisory) / CI-time (enforced) | The compiler itself does not reject unpinned `uses:` references; SHA-pinning is enforced by CI tooling (`actionlint`, `poutine`, `zizmor`) run against compiled `.lock.yml` files, not by the compiler at `gh aw compile` time (see Appendix G.1 coverage gap) | | ||
| | BP-05 Enable threat detection | Compile-time (job generation) / Runtime (detection execution) | The compiler generates a `detection` job when `threat-detection.enabled` is not explicitly `false`; the actual AI-based/TruffleHog scan and the `needs.detection.outputs.success` gate on `safe_outputs` are evaluated at runtime | |
|
|
||
| #### Example 5: Pre-Activation, Detection, and Conclusion Job Structure | ||
|
|
||
| The following excerpt (abridged from a compiled `.lock.yml`) shows the three implementation-detail jobs referenced in the Minor Discrepancies section of `specs/security-architecture-spec-validation.md`: `pre_activation` (role-based access control ahead of `activation`), `detection` (runtime manifestation of the Threat Detection Layer, Section 9), and `conclusion` (cleanup/summary reporting job that always runs). |
| # ... run summary, cleanup, and failure reporting ... | ||
| ``` | ||
|
|
||
| **Behavior**: `pre_activation` gates `activation` on role membership; `detection` runs after `agent` and gates `safe_outputs` on a successful threat-detection conclusion; `conclusion` always runs last (subject to `always()`) to report status regardless of upstream success or failure. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Daily SPDD review (rotation indices 10–14) flagged several REASONS-canvas gaps and stale sync/validation debts across
specs/safe-output-outcome-evaluation.md,specs/security-architecture-spec-summary.md,specs/security-architecture-spec-validation.md,specs/security-architecture-spec.md, andscratchpad/github-mcp-access-control-specification.md.Re-validation & discrepancy closure (
specs/security-architecture-spec-validation.md)repos/allowed-reposdivergence): deferred, since the companion spec already resolved it as a documented deprecated alias with no behavioral change, andTestValidateGitHubGuardPolicyalready covers both spellings.sanitize_content_core.cjs,checkout_pr_branch.cjs) still exist.New example (
specs/security-architecture-spec.md).lock.ymlexcerpt showingpre_activation→detection→safe_outputs→conclusionjob wiring.Structure mapping (
specs/safe-output-outcome-evaluation.md)Added a "Structure" subsection mapping each of the 30 numbered safe-output-type sections to its implementing Go file under
pkg/workflow/, e.g.:create_pull_requestpkg/workflow/create_pull_request.goclose_issuepkg/workflow/close_entity_helpers.goSafeguards table (
specs/security-architecture-spec-summary.md)§8.5 / §C.4 tightening (
scratchpad/github-mcp-access-control-specification.md)P5_NotBlocked/P6_IntegrityMetpredicate names used in §11 andgithub_mcp_access_control_formal_test.go, removing ambiguity about which prose step maps to which guard predicate/error code.403s from non-retryable permission-denial403s, and fail-closed behavior on exhausted retries.No production code changed — all edits are to specification/scratchpad markdown.