Implement discussion outcome evaluators and sync security architecture evidence#45784
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds discussion outcome evaluation, PM-11 formal coverage, and updated security evidence documentation.
Changes:
- Implements and tests discussion evaluators.
- Adds PM-11 compilation coverage.
- Updates security validation and access-control specifications.
Show a summary per file
| File | Description |
|---|---|
actions/setup/js/evaluate_outcomes.cjs |
Adds discussion evaluation logic. |
actions/setup/js/evaluate_outcomes.test.cjs |
Tests discussion outcomes. |
pkg/workflow/security_architecture_sg_formal_test.go |
Adds PM-11 formal test. |
specs/safe-output-outcome-evaluation.md |
Updates evaluator matrix. |
specs/security-architecture-spec-summary.md |
Records PM-11 coverage. |
specs/security-architecture-spec-validation.md |
Updates security evidence. |
scratchpad/github-mcp-access-control-specification.md |
Tightens fail-closed requirements. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 7/7 changed files
- Comments generated: 10
- Review effort level: Medium
| const answered = discussion.answer_chosen_at != null || discussion.answer != null || discussion.answered === true; | ||
| if (answered) { | ||
| out.result = "accepted"; | ||
| out.outcome_status = "accepted"; | ||
| out.evidence_strength = "strong"; | ||
| out.signal = "answered"; | ||
| out.detail = "answered"; | ||
| return out; | ||
| } | ||
|
|
||
| if (discussion.locked === true) { | ||
| out.result = "rejected"; | ||
| out.outcome_status = "rejected"; | ||
| out.evidence_strength = "strong"; | ||
| out.signal = "locked"; | ||
| out.detail = "locked"; | ||
| return out; | ||
| } |
| | `close_discussion` | implemented | `pkg/workflow/safe_outputs_dispatch.go`, `pkg/cli/outcome_eval.go` (`evalCloseDiscussion`) | `actions/setup/js/close_discussion.cjs`, `actions/setup/js/evaluate_outcomes.cjs` (`evaluateCloseDiscussion`) | | ||
| | `create_discussion` | implemented | `pkg/workflow/safe_outputs_dispatch.go`, `pkg/cli/outcome_eval.go` (`evalCreateDiscussion`) | `actions/setup/js/create_discussion.cjs`, `actions/setup/js/evaluate_outcomes.cjs` (`evaluateCreateDiscussion`) | |
| // ThreatDetectionOrDefault → TestFormalThreatDetection_EnabledByDefault | ||
| // ThreatDetectionOrDefault → TestFormalThreatDetection_ExplicitDisable | ||
| // IsContinueOnError → TestFormalThreatDetection_ContinueOnErrorDefault | ||
| // PM11_PreActivationMembership → TestFormalPM11_PreActivationContainsMembershipStep |
|
|
||
| - **T-OI-003 / T-OI-004 — safe output type support and validation rules**: `actions/setup/js/collect_ndjson_output.test.cjs` exercises `create_discussion` schema handling (required fields, min/max handling, mixed-type batches), while `pkg/workflow/compiler_safe_outputs_steps.go` wires agent output download plus the single `Process Safe Outputs` dispatcher step that performs validation before any write action is applied. | ||
| - **T-OI-005 — token precedence**: `pkg/workflow/github_token.go` documents and implements the checkout/push token precedence chain, and `pkg/workflow/github_token_test.go` verifies the ordering (per-output PAT overrides checkout-scoped safe-output app token, which overrides safe-outputs level fallbacks). `pkg/workflow/compiler_safe_outputs_steps.go` then persists that resolved token into checkout credentials and the `GITHUB_TOKEN` environment for trusted safe-output handlers only. | ||
| - **T-OI-006 — token secret-expression handling**: `pkg/workflow/github_token.go` emits GitHub Actions secret expressions (`${{ secrets... }}`) instead of raw values for safe-output tokens, and `pkg/workflow/safe_outputs_validation.go` rejects configurations that would attempt workflow-scoped writes without a GitHub App. |
| - **T-NI-008**: representative compiled workflows such as `.github/workflows/portfolio-analyst.lock.yml` run the agent inside AWF with an explicit `--mcp-config "${RUNNER_TEMP}/gh-aw/mcp-config/mcp-servers.json"` argument and sandbox mounts rooted under `${RUNNER_TEMP}/gh-aw`, evidencing that MCP access is routed through the sandbox/firewall wrapper rather than direct host execution. | ||
| - **T-NI-009**: the `GH_AW_ALLOWED_DOMAINS` environment built in `pkg/workflow/compiler_safe_outputs_steps.go` feeds the same allowlist into runtime safe-output sanitization, closing the gap between network policy and content filtering. | ||
|
|
||
| **Status**: ✅ **VERIFIED** — the validation report now includes evidence for T-NI-001 through T-NI-009. |
| - **T-SI-006**: the compiled AWF command mounts only the staged `${RUNNER_TEMP}/gh-aw` tool/config tree and passes MCP configuration through that sandboxed mount, rather than exposing arbitrary host state to the agent container. | ||
| - **T-SI-007**: the representative detection job uses a separate AWF invocation (including the same chroot patch path) even when host access is explicitly enabled for detection, showing that sandboxing and network policy remain composed controls rather than mutually exclusive modes. | ||
|
|
||
| **Status**: ⚠️ **PARTIALLY EVIDENCED** — compiled lock-file and runtime-script evidence now cover all T-SI identifiers at least partially, but this report still lacks a direct runtime probe demonstrating host/socket invisibility from inside the sandbox. |
| | 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 | | ||
| | Output Isolation | T-OI-001 to T-OI-007 | ✅ EVIDENCED | OI-01/OI-06 remain verified, and §8a now adds dedicated evidence for T-OI-003 through T-OI-007 (type coverage, validation rules, token precedence, secret-expression handling, and write-job isolation) | | ||
| | Network Isolation | T-NI-001 to T-NI-009 | ✅ EVIDENCED | §8 and §8b now cover AWF installation, ecosystem/domain validation, protocol filtering, blocked-domain precedence, MCP sandbox routing, and allowlist-driven content sanitization | |
| 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. | ||
| 1. **Sandbox Isolation (T-SI-001 to T-SI-007)** — Reduced from full gap to partial evidence in §8c. Remaining work is a direct runtime probe for host/socket visibility from inside the AWF sandbox. |
| - ✅ Role-based access control (pre_activation with membership checks) | ||
| - ✅ PM-11 formal coverage (`TestFormalPM11_PreActivationContainsMembershipStep`) | ||
| - ✅ Output isolation evidence for handler coverage, token precedence, and write-job separation | ||
| - ✅ Network isolation evidence for allowlist validation, protocol filtering, ecosystem expansion, and MCP/firewall wrapping |
| return out; | ||
| } | ||
|
|
||
| if (discussion.locked === true) { |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #45784 does not have the 'implementation' label and has ≤100 new lines of code in business logic directories (45 additions detected). |
|
✅ PR Code Quality Reviewer completed the code quality review. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠\n\nApplied /tdd, /codebase-design, and /grill-with-docs — requesting changes on correctness and spec-accuracy issues.\n\n\n📋 Key Themes & Highlights
\n\n#### Key Themes\n\n- Evaluator logic bug: evaluateCreateDiscussion checks answered before locked, so a discussion that is both answered and locked is accepted, contradicting the stated semantics (locked → rejected).\n- Spec overclaims: specs/safe-output-outcome-evaluation.md marks close_discussion and create_discussion as implemented but the Go evaluators (pkg/cli/outcome_eval_generic.go) still return OutcomePending, failing the document's own implemented definition.\n- Stale invariant count: security_architecture_sg_formal_test.go header comments say eight supporting invariants; PM-11 makes nine.\n- Validation doc overclaims: T-NI, T-OI, and T-SI entries cite files that do not demonstrate the required behavior, upgrading status beyond what the cited code supports.\n- Missing spec entry: The rejected outcome for close_discussion is absent from specs/safe-output-outcome-evaluation.md.\n\n#### Positive Highlights\n\n- ✅ Good test coverage for the new JS evaluators — happy paths and key edge cases present.\n- ✅ Clean evaluateItem dispatch extension, consistent with existing patterns.\n- ✅ Minimal, well-targeted URL regex extension for discussions/.\n\n
📋 Key Themes & Highlights
\n\n#### Key Themes\n\n- Evaluator logic bug:evaluateCreateDiscussion checks answered before locked, so a discussion that is both answered and locked is accepted, contradicting the stated semantics (locked → rejected).\n- Spec overclaims: specs/safe-output-outcome-evaluation.md marks close_discussion and create_discussion as implemented but the Go evaluators (pkg/cli/outcome_eval_generic.go) still return OutcomePending, failing the document's own implemented definition.\n- Stale invariant count: security_architecture_sg_formal_test.go header comments say eight supporting invariants; PM-11 makes nine.\n- Validation doc overclaims: T-NI, T-OI, and T-SI entries cite files that do not demonstrate the required behavior, upgrading status beyond what the cited code supports.\n- Missing spec entry: The rejected outcome for close_discussion is absent from specs/safe-output-outcome-evaluation.md.\n\n#### Positive Highlights\n\n- ✅ Good test coverage for the new JS evaluators — happy paths and key edge cases present.\n- ✅ Clean evaluateItem dispatch extension, consistent with existing patterns.\n- ✅ Minimal, well-targeted URL regex extension for discussions/.\n\n🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 23.4 AIC · ⌖ 6.83 AIC · ⊞ 6.7K
Comment /matt to run again
There was a problem hiding this comment.
Review Summary
The discussion evaluator implementations have several blocking issues.
Critical (blocking):
-
evaluateCreateDiscussionanswered/locked precedence bug (line 844 ofevaluate_outcomes.cjs): A discussion with bothanswer_chosen_atset andlocked: trueis classified asacceptedbecause the answered check runs before the locked check. If locked discussions should berejected, thelockedcheck must run first. -
Spec/implementation mismatch (
specs/safe-output-outcome-evaluation.mdline 132): The rows forclose_discussionandcreate_discussionare markedimplemented, but the Go evaluators inpkg/cli/outcome_eval_generic.go:49-68still returnOutcomePendingwithout discussion evaluation. The spec definition ofimplementedrequires a working Go-side evaluator. -
Missing
rejectedoutcome in spec (line 837 ofevaluate_outcomes.cjs):evaluateCreateDiscussionintroduces arejectedresult for locked discussions, butspecs/safe-output-outcome-evaluation.mdsection 11 only listsacceptedandignored. The normative spec must be updated.
Non-blocking:
-
Stale header (
security_architecture_sg_formal_test.goline 23): Adding PM-11 increases invariant count from 8 to 9; update the comment. -
Overstated evidence (
security-architecture-spec-validation.md): Multiple T-NI and T-SI sections upgraded to VERIFIED cite only syntax-acceptance validators, not network modes, ecosystem expansion, allowlist matching, or sandbox isolation behaviors. Downgrade to partial evidence.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 19 AIC · ⌖ 4.4 AIC · ⊞ 5K
There was a problem hiding this comment.
REQUEST_CHANGES — Two blocking correctness issues in the JS evaluators
Both new evaluators (evaluateCloseDiscussion, evaluateCreateDiscussion) will silently fail in every production run due to an incorrect API endpoint, and the evaluateCreateDiscussion guard ordering produces wrong outcomes for answered+locked discussions.
Blocking findings
1. Wrong API endpoint (critical) — GitHub Discussions have no REST endpoint at repos/{owner}/{repo}/discussions/{number}. Both evaluators call that path via ghAPI, which will return 404/null in production, causing every evaluation to exit with result: "unknown" / detail: "api error". Tests pass only because mockAPI is a stub that accepts arbitrary keys. This also invalidates the spec table upgrade to implemented.
2. answered/locked guard order (high) — In evaluateCreateDiscussion, the answered check (line 134) runs before locked (line 144). A discussion that is both answered and locked is classified as accepted instead of rejected. Existing comment by @Copilot (#3589283873) confirms this; the test suite has no case exercising the overlap.
Previously flagged issues (carry-over from prior review round)
The 10 comments from the prior review round remain unresolved — in particular the Go evaluator OutcomePending stubs, the stale file header comment, the overstated T-NI/T-SI compliance matrix, and the missing rejected case in the spec discussion-semantics table. Those must also be addressed before merge.
🔎 Code quality review by PR Code Quality Reviewer · 60.6 AIC · ⌖ 5.04 AIC · ⊞ 5.6K
Comment /review to run again
Comments that could not be inline-anchored
actions/setup/js/evaluate_outcomes.cjs:46
Runtime failure: GitHub Discussions require GraphQL, not REST. The api(repos/${repo}/discussions/${number}) call will always fail in production — GitHub has no REST endpoint at that path; discussions are only accessible via the GraphQL API.
<details>
<summary>💡 Details and suggested fix</summary>
GitHub's REST API returns 404 for GET /repos/{owner}/{repo}/discussions/{number}. The existing evaluateCloseIssue and evaluateClosePullRequest work because issues and pulls have RE…
actions/setup/js/evaluate_outcomes.cjs:134
Priority inversion: answered check precedes locked check, producing wrong accepted result. A discussion that is both answered and locked returns accepted even though locked discussions are defined as rejected.
<details>
<summary>💡 Suggested fix</summary>
Reorder the guards so locked is checked first:
if (discussion.locked === true) {
out.result = 'rejected';
out.outcome_status = 'rejected';
out.evidence_strength = 'strong';
out.signal = 'locked';
out.detail = 'lo…
</details>
<details><summary>actions/setup/js/evaluate_outcomes.cjs:47</summary>
**Inconsistent API error guard: `evaluateCloseDiscussion` checks for malformed objects but `evaluateCreateDiscussion` does not.** A non-null but semantically empty API response (e.g. `{}`) will be silently processed by `evaluateCreateDiscussion` producing wrong results, while `evaluateCloseDiscussion` would correctly reject it.
<details>
<summary>💡 Details</summary>
`evaluateCloseDiscussion` (line 47) guards with:
```js
if (!discussion || (typeof discussion.state !== 'string' && typeof discu…
</details>
<details><summary>pkg/workflow/security_architecture_sg_formal_test.go:424</summary>
**Fragile YAML quoting assertion will silently break on serializer upgrades.** The assertion `assert.Contains(t, preActivationSection, `GH_AW_REQUIRED_ROLES: "write"`)` hardcodes the double-quoted YAML serialization of a plain string. gopkg.in/yaml.v2 and yaml.v3 handle string quoting differently, and a serializer version bump can emit `GH_AW_REQUIRED_ROLES: write` (unquoted), causing the test to fail with a misleading 'no membership gate' diagnostic.
<details>
<summary>💡 Suggested fix</summa…
</details>
<details><summary>scratchpad/github-mcp-access-control-specification.md:439</summary>
**Normative spec conflates 403 permission-denied with 403 rate-limit — these are distinct failure modes.** The new text says `On `403` or `429` responses that indicate GitHub rate limiting` — but GitHub returns 403 for many reasons beyond rate limiting (insufficient permissions, OAuth scope, suspended account, etc.). Treating all 403s under rate-limit guidance can cause implementations to retry legitimate permission denials.
<details>
<summary>💡 Suggested fix</summary>
Distinguish by respons…
</details>
🧪 Test Quality Sentinel Report✅ Test Quality Score: 96/100 — Excellent
📊 Metrics (7 tests)
Verdict
References: Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"See Network Configuration for more information.
|
This updates the remaining gaps from the 2026-07-15 SPDD spec plan: the safe-output outcome evaluator matrix was out of sync with runtime behavior, PM-11 lacked dedicated formal coverage, and the security validation/spec docs still treated several output/network/sandbox controls as unevidenced or underspecified.
Outcome evaluation runtime
close_discussionandcreate_discussioninactions/setup/js/evaluate_outcomes.cjs.push_to_pull_request_branchas runtime-is and aligned the spec to the already-implemented evaluator.Discussion outcome semantics
close_discussionnow accepts only closed discussions and rejects reopened/open discussions.create_discussionnow classifies answered or replied-to discussions as accepted, locked discussions as rejected, and no-engagement discussions as ignored.Formal security coverage
TestFormalPM11_PreActivationContainsMembershipSteptopkg/workflow/security_architecture_sg_formal_test.go.pre_activationjob containing thecheck_membershipstep and required role wiring.Spec and validation sync
specs/safe-output-outcome-evaluation.mdto markclose_discussion,create_discussion, andpush_to_pull_request_branchas implemented with dedicated JS mappings.specs/security-architecture-spec-summary.mdto add PM-11 to the behavioral coverage map and sync the formal suite count.specs/security-architecture-spec-validation.mdto:MCP access-control safeguards
scratchpad/github-mcp-access-control-specification.mdwith normative fail-closed requirements for403/429and5xxauthorization paths.