Close SPDD daily spec-review action items for security-architecture and access-control specs

scratchpad/github-mcp-access-control-specification.md

@@ -1597,19 +1597,19 @@ ELSE:
 
 ### 8.5 Combined Evaluation Order
 
-The complete integrity evaluation MUST occur in this order:
+The complete integrity evaluation MUST occur in this order. Each step is labeled with the formal guard-predicate name used in §11 Compliance Testing and `pkg/workflow/github_mcp_access_control_formal_test.go` (`formalEvaluateAccess`) so that the two are unambiguously the same evaluation sequence:
 
 ```text
-1. Author Check (blocked-users)
-   → If author is blocked: DENY immediately
-2. Label Promotion (approval-labels)
+1. Author Check — predicate P5_NotBlocked (blocked-users)
+   → If author is blocked: DENY immediately (error code -32005)
+2. Label Promotion — integrity computation input to P6 (approval-labels), not a standalone gating predicate
    → Promote effective_integrity if a matching label is present
-3. Threshold Check (min-integrity)
-   → If effective_integrity < min-integrity: DENY
+3. Threshold Check — predicate P6_IntegrityMet (min-integrity)
+   → If effective_integrity < min-integrity: DENY (error code -32006)
 4. All integrity checks passed: ALLOW
 ```
 
-This order ensures that blocked users can never be promoted by labels, and that label promotion is always considered before the threshold check.
+This order ensures that blocked users can never be promoted by labels, and that label promotion is always considered before the threshold check. Concretely: P5_NotBlocked (step 1) is evaluated and MUST fire before P6_IntegrityMet (step 3); label promotion (step 2) only mutates the `effective_integrity` value consumed by P6_IntegrityMet and never overrides a P5_NotBlocked denial. Note that P5_NotBlocked and P6_IntegrityMet are themselves evaluated after the repository/role/visibility predicates P1–P4 in §4.5.3's guard evaluation order; §8.5 covers only the internal ordering of the two integrity-related predicates.
 
 ---
 
@@ -2680,6 +2680,53 @@ GitHub API rate limits apply to:
 - Monitor rate limit consumption
 - Implement exponential backoff for rate limit errors
 
+**Example: Exponential backoff for rate-limited permission/visibility queries**
+
+Consistent with the fail-closed requirement in §9.4 (Security Considerations), a `403`/`429` response during backoff MUST still deny the current access-control decision rather than substituting a stale cached "allow" result while a retry is pending.
+
+```text
+function queryWithBackoff(request):
+    maxAttempts = 5
+    baseDelayMs = 500      # initial delay
+    maxDelayMs  = 30000    # cap to avoid unbounded waits
+
+    for attempt in 1..maxAttempts:
+        response = performGitHubAPIRequest(request)
+
+        # 429 is always a primary rate limit. A 403 with a rate-limit-related
+        # body/header (e.g. "secondary rate limit" or a "Retry-After" header)
+        # is GitHub's secondary rate limit and is retried the same way; a 403
+        # WITHOUT rate-limit signals is a permission/authorization denial and
+        # MUST NOT be retried — return it immediately as a non-retryable error.
+        isRateLimited = response.status == 429 or
+            (response.status == 403 and isRateLimitSignal(response))
+
+        if isRateLimited:
+            if attempt == maxAttempts:
+                # Fail closed: no more retries — deny for this decision (§9.4)
+                return DENY_RATE_LIMITED
+
+            retryAfter = response.headers["Retry-After"]  # seconds, if present
+            if retryAfter is present:
+                delayMs = retryAfter * 1000
+            else:
+                # Exponential backoff with jitter: base * 2^(attempt-1), capped
+                delayMs = min(baseDelayMs * (2 ** (attempt - 1)), maxDelayMs)
+                delayMs = delayMs + randomJitterMs(0, delayMs * 0.1)
+
+            sleep(delayMs)
+            continue
+
+        return response  # success, or a non-rate-limit error (e.g. plain 403); caller handles normally
+    # Loop only reaches maxAttempts via the rate-limited branch above, which
+    # always returns DENY_RATE_LIMITED on the final attempt, so this point is unreachable.
+```
+
+**Notes**:
+- Respect the `Retry-After` header when GitHub provides one; fall back to exponential backoff (base 500ms, cap 30s) with jitter otherwise.
+- Treat `403` as retryable only when it carries GitHub's secondary-rate-limit signal (`Retry-After` header or a rate-limit message in the response body); an ordinary `403` permission denial MUST be returned immediately, not retried.
+- Exhausting `maxAttempts` MUST result in a fail-closed denial for the current request, not an implicit allow.
+
 #### C.5 Configuration Validation Timing
 
 **Compilation-Time Validation**: Catches most configuration errors before runtime

specs/safe-output-outcome-evaluation.md

@@ -815,6 +815,45 @@ The table below specifies one conformance test row per safe-output type. Each ro
 | §29 | `missing_tool` | `pkg/cli/outcome_eval_test.go` | covered |
 | §30 | `replace_label` | `pkg/cli/outcome_eval_update_test.go`, `pkg/workflow/replace_label_formal_test.go` | covered |
 
+### Structure: Safe-Output Section-to-Implementation Mapping
+
+Each numbered safe-output-type section above corresponds to a dedicated configuration/compilation implementation file under `pkg/workflow/`. This mapping is distinct from the Compliance test mapping above; it maps specification sections to the Go source that defines and compiles the output type (shared cross-cutting logic such as `safe_output_handlers.go` and `compiler_safe_outputs_job.go` is omitted for brevity since it applies to all types).
+
+| Section | Output type | Implementation file(s) |
+|---|---|---|
+| §1 | `create_pull_request` | `pkg/workflow/create_pull_request.go` |
+| §2 | `create_issue` | `pkg/workflow/create_issue.go` |
+| §3 | `add_comment` | `pkg/workflow/add_comment.go` |
+| §4 | `add_labels` | `pkg/workflow/add_labels.go` |
+| §5 | `add_reviewer` | `pkg/workflow/add_reviewer.go` |
+| §6 | `update_issue` | `pkg/workflow/update_issue.go` |
+| §7 | `update_pull_request` | `pkg/workflow/update_pull_request.go` |
+| §8 | `close_issue` | `pkg/workflow/close_entity_helpers.go` |
+| §9 | `close_pull_request` | `pkg/workflow/close_entity_helpers.go` |
+| §10 | `close_discussion` | `pkg/workflow/close_entity_helpers.go` |
+| §11 | `create_discussion` | `pkg/workflow/create_discussion.go` |
+| §12 | `update_discussion` | `pkg/workflow/update_discussion.go` |
+| §13 | `create_pull_request_review_comment` | `pkg/workflow/create_pr_review_comment.go` |
+| §14 | `submit_pull_request_review` | `pkg/workflow/submit_pr_review.go` |
+| §15 | `reply_to_pull_request_review_comment` | `pkg/workflow/reply_to_pr_review_comment.go` |
+| §16 | `resolve_pull_request_review_thread` | `pkg/workflow/resolve_pr_review_thread.go` |
+| §17 | `push_to_pull_request_branch` | `pkg/workflow/push_to_pull_request_branch.go`, `pkg/workflow/push_to_pull_request_branch_validation.go` |
+| §18 | `mark_pull_request_as_ready_for_review` | `pkg/workflow/mark_pull_request_as_ready_for_review.go` |
+| §19 | `assign_to_agent` | `pkg/workflow/assign_to_agent.go` |
+| §20 | `dispatch_workflow` | `pkg/workflow/dispatch_workflow.go`, `pkg/workflow/dispatch_workflow_validation.go`, `pkg/workflow/dispatch_workflow_file_resolver.go` |
+| §21 | `autofix_code_scanning_alert` | `pkg/workflow/autofix_code_scanning_alert.go` |
+| §22 | `create_code_scanning_alert` | `pkg/workflow/create_code_scanning_alert.go` |
+| §23 | `link_sub_issue` | `pkg/workflow/link_sub_issue.go` |
+| §24 | `hide_comment` | `pkg/workflow/hide_comment.go` |
+| §25 | `assign_milestone` | `pkg/workflow/assign_milestone.go` |
+| §26 | `update_project` | `pkg/workflow/update_project.go` |
+| §27 | `update_release` | `pkg/workflow/update_release.go` |
+| §28 | `noop` | `pkg/workflow/noop.go` |
+| §29 | `missing_tool` | `pkg/workflow/missing_issue_reporting.go` |
+| §30 | `replace_label` | `pkg/workflow/replace_label.go` |
+
+Sync procedure: when a safe-output type's implementation file is renamed, split, or removed, update the corresponding row in this table in the same change that moves the code.
+
 ### OTel Backend Unavailability
 
 When the OTLP exporter is unavailable (e.g., endpoint unreachable, network timeout, authentication failure) during outcome evaluation, the following safeguards **MUST** apply:

specs/security-architecture-spec-summary.md

@@ -264,6 +264,17 @@ Step-by-step checklist for verifying that a compiled `.lock.yml` file meets all
 ### Appendix H: Security Best Practices
 Six key best practices with "Don't" and "Do" examples.
 
+**Safeguards — compile-time vs. runtime enforcement**:
+
+| 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 |
+| BP-06 Role-based access control | Runtime | `roles:` configures the `GH_AW_REQUIRED_ROLES` environment variable consumed by the `pre_activation` job's `check_membership.cjs` step, which queries GitHub's API for the triggering actor's role at workflow run time; the compiler does not verify roles ahead of time |
+
 ## Target Audience
 
 - **Security Engineers**: Audit and verify security controls

specs/security-architecture-spec-validation.md

@@ -580,23 +580,23 @@ concurrency:
 
 **Clarification**: `pre_activation` handles role-based access control before activation. This is an implementation detail that doesn't contradict the specification - it's an additional security layer.
 
-**Recommendation**: Consider adding a note about role validation occurring in a separate pre-activation step.
+**Status**: ✅ **CLOSED** — Appendix D of `specs/security-architecture-spec.md` (Example 5) now shows `pre_activation` as a separate job gating `activation` on role membership.
 
 ### 2. Detection Job Naming
 
 **Observation**: The specification mentions "Threat Detection Layer" but doesn't explicitly state it as a separate job named `detection`.
 
 **Clarification**: The `detection` job is the runtime manifestation of the threat detection layer described in Section 9.
 
-**Recommendation**: Add example job structure showing `detection` as a separate job in Appendix D.
+**Status**: ✅ **CLOSED** — Appendix D of `specs/security-architecture-spec.md` (Example 5) now shows `detection` as a separate job between `agent` and `safe_outputs`.
 
 ### 3. Conclusion Job
 
 **Observation**: Lock files contain a `conclusion` job not mentioned in the specification.
 
 **Clarification**: The `conclusion` job is an implementation detail for workflow cleanup and summary generation.
 
-**Recommendation**: Consider adding a note about optional cleanup/reporting jobs.
+**Status**: ✅ **CLOSED** — Appendix D of `specs/security-architecture-spec.md` (Example 5) now shows `conclusion` as the terminal `always()` job for cleanup/status reporting.
 
 ---
 
@@ -613,6 +613,14 @@ A conforming maintainer MUST re-run this validation when any of the following oc
 
 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.
 
+### Trigger #3 Decision Log
+
+**2026-08-13 — `repos` vs `allowed-repos` field-naming divergence (deferred, no re-validation needed)**
+
+`scratchpad/github-mcp-access-control-specification.md`'s Divergence Audit (§Sync Notes) flagged that §4.4.1 historically used `repos` as the field name while §4.1 and the guard-policies spec used `allowed-repos`. This was investigated as a potential trigger-#3 event (companion guard-policy spec revision).
+
+**Decision: Defer — no re-validation of this report required.** The companion spec's own Divergence Audit already marked the item **Resolved**: `allowed-repos` is the canonical frontmatter key, `repos` is a documented deprecated alias for backward compatibility (`pkg/workflow/mcp_github_config.go` lines ~322-335), and `pkg/workflow/tools_validation_github.go` validates both spellings identically via the shared `AllowedRepos` field. This is a terminology/alias clarification, not a change to guard-policy *behavior*, job structure, or security guarantees. It does not affect the §3.2 Security Guarantees or §9 Threat Detection claims in `specs/security-architecture-spec.md`, so the Specification Accuracy Summary table below requires no update. `go test ./pkg/workflow/ -run TestValidateGitHubGuardPolicy` already exercises both the `repos` alias and `allowed-repos` field name (see `pkg/workflow/tools_validation_test.go`) and passes.
+
 ### Failure Escalation
 
 When a re-validation pass identifies a specification claim that is no longer verifiable against the implementation:

specs/security-architecture-spec.md

@@ -1584,6 +1584,68 @@ safe-outputs:
 
 **Behavior**: All workflow runs complete in order, preventing incomplete operations.
 
+#### 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).
+
+```yaml
+jobs:
+  pre_activation:
+    if: >
+      (github.event_name != 'pull_request' || github.event.pull_request.head.repo.id == github.repository_id)
+    runs-on: ubuntu-slim
+    permissions:
+      contents: read
+    outputs:
+      activated: ${{ steps.check_membership.outputs.is_team_member == 'true' }}
+    steps:
+      - name: Check team membership for workflow
+        id: check_membership
+        uses: actions/github-script@<pinned-sha> # vX.Y.Z
+        env:
+          GH_AW_REQUIRED_ROLES: "admin,maintainer,write"
+
+  activation:
+    needs: [pre_activation]
+    # ... timestamp/lockdown validation (Section 11.1) ...
+
+  agent:
+    needs: [activation]
+    permissions:
+      contents: read
+    # ... agentic execution (read-only) ...
+
+  detection:
+    needs: [activation, agent]
+    if: always() && needs.agent.result != 'skipped'
+    runs-on: ubuntu-latest
+    permissions:
+      contents: read
+    outputs:
+      detection_conclusion: ${{ steps.detection_conclusion.outputs.conclusion }}
+      detection_success: ${{ steps.detection_conclusion.outputs.success }}
+    # ... AI threat-detection scan of agent output (Section 9.1) ...
+
+  safe_outputs:
+    needs: [activation, agent, detection]
+    if: (!cancelled()) && needs.agent.result != 'skipped' && needs.detection.result == 'success'
+    permissions:
+      issues: write
+      pull-requests: write
+    # ... write operations gated on successful detection ...
+
+  conclusion:
+    needs: [activation, agent, detection, safe_outputs]
+    if: always() && (needs.agent.result != 'skipped' || needs.activation.outputs.lockdown_check_failed == 'true')
+    permissions:
+      actions: read
+      issues: write
+      pull-requests: write
+    # ... 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.
+
 ### Appendix E: Concurrency Control Examples
 
 #### Example 1: Pull Request Workflow with Cancellation