Narrow submit_pull_request_review event enum to allowed-events config#54362
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
❌ Ponytail Reviewer failed. Please review the logs for details. Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details.
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #54362 does not have the 'implementation' label and has only 56 new lines of code in business logic directories (threshold: 100).
|
Comment MemoryNote This comment is managed by comment memory.It stores persistent context for this thread in the code block at the top of this comment.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict
REQUEST_CHANGES — the schema narrowing is headed in the right direction, but it still trusts AllowedEvents too much at generation time and the added tests miss the failure mode.
The blocking theme
computePropertyInjectionsnow copiesSubmitPullRequestReview.AllowedEventsstraight into the generated schema without re-validating it.- That assumption only holds if every construction path for
SafeOutputsConfiggoes throughparseSubmitPullRequestReviewConfig, but this package already has direct struct-based tests and helpers, so future callers can feed invalid values into code generation and emit an invalid or misleading schema. - The new tests only cover the happy path and empty config, so they would not catch that regression.
🔎 Code quality review by PR Code Quality Reviewer · gpt54 · 12.3 AIC · ⌖ 6.83 AIC · ⊞ 7K
Comment /review to run again
There was a problem hiding this comment.
The change is correct and well-tested. Narrowing the event enum at schema generation time prevents the agent from selecting a disallowed review decision while keeping runtime enforcement as defense-in-depth. Tests cover single event, multiple events, and the no-config no-op case.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 14.3 AIC · ⌖ 8.78 AIC · ⊞ 5.7K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — approving with one minor suggestion.
📋 Key Themes & Highlights
Key Themes
- Root cause fixed at the right layer: The event enum is now narrowed at schema-generation time so the agent can never select a disallowed event in the first place. Runtime enforcement remains as defense-in-depth. This is the correct fix — eliminating the impossible call, not just rejecting it later.
- Established pattern followed: The implementation mirrors the existing
close_issue→state_reasoninjection exactly, keeping the codebase consistent. - Test coverage is solid: Three cases cover single event, multiple events, and the no-config default. One gap: a
nil-config test (see inline comment) would make the suite symmetric withclose_issue.
Positive Highlights
- ✅ Defense-in-depth preserved — runtime check in
submit_pr_review.cjsuntouched - ✅ No JS changes needed; the generic
property_injectionspath picks up the new injection automatically - ✅ Description in the
eventschema field explains why the enum is narrowed, which helps agent introspection
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 24.1 AIC · ⌖ 9.95 AIC · ⊞ 7.8K
Comment /matt to run again
|
|
||
| // TestComputePropertyInjectionsNoAllowedEventsSubmitPRReview verifies that no injection | ||
| // happens when allowed-events is not configured, so the static schema's full enum applies. | ||
| func TestComputePropertyInjectionsNoAllowedEventsSubmitPRReview(t *testing.T) { |
There was a problem hiding this comment.
[/tdd] The close_issue nil-config test (TestComputePropertyInjectionsNilCloseIssues) covers nil config explicitly — adding the same for submit_pull_request_review would mirror the pattern and document the nil contract.
💡 Suggested addition
func TestComputePropertyInjectionsNilSubmitPRReview(t *testing.T) {
injections := computePropertyInjections(&SafeOutputsConfig{
SubmitPullRequestReview: nil,
})
assert.NotContains(t, injections, "submit_pull_request_review")
}Low-risk omission — the != nil guard already handles this — but the symmetry with close_issue tests makes the contract explicit.
@copilot please address this.
There was a problem hiding this comment.
Pull request overview
Narrows generated PR-review tool schemas to configured review events.
Changes:
- Injects configured
allowed-eventsinto theeventenum. - Adds tests for single, multiple, and default event sets.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/safe_outputs_tools_generation.go |
Generates the restricted event schema. |
pkg/workflow/safe_outputs_tools_generation_test.go |
Tests event-enum generation. |
Review details
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Balanced
| if safeOutputs.SubmitPullRequestReview != nil && len(safeOutputs.SubmitPullRequestReview.AllowedEvents) > 0 { | ||
| allowedEvents := safeOutputs.SubmitPullRequestReview.AllowedEvents | ||
| injections["submit_pull_request_review"] = map[string]any{ | ||
| "event": map[string]any{ | ||
| "type": "string", | ||
| "enum": allowedEvents, |
🔍 PR TriageCategory: Score: 44/100 (impact 18/50 + urgency 10/30 + quality 16/20) Recommended action: Small schema-tightening change with tests, CI passing. Automated triage — run 32432526976
|
|
@copilot Quick triage nudge for PR #54362.
Run: https://github.com/github/gh-aw/actions/runs/32434686968
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed the blocking review feedback in commit I updated schema generation so |
submit-pull-request-review.allowed-eventswas only enforced at runtime — the generatedsubmit_pull_request_reviewMCP tool schema still advertised all three event values (APPROVE,REQUEST_CHANGES,COMMENT), letting the agent pick a disallowed event. Runtime correctly rejected it, but this got misclassified as prompt injection by threat detection and failed the workflow instead of just posting the review.Compiler change
computePropertyInjections(pkg/workflow/safe_outputs_tools_generation.go) now narrows theeventfield'senumto the configuredallowed-eventslist, following the same pattern already used forclose_issue'sstate_reasoninjection.submit_pr_review.cjsis unchanged and remains as defense in depth.No JS changes were required: the existing generic
property_injectionsmechanism ingenerate_safe_outputs_tools.cjs(already used forclose_issue) picks up the new injection automatically.For example, with:
the generated tool schema now has
"event": {"enum": ["COMMENT"]}instead of["APPROVE", "REQUEST_CHANGES", "COMMENT"], so the agent can no longer select an event that will inevitably be rejected.Tests
pkg/workflow/safe_outputs_tools_generation_test.gocovering single/multiple allowed events and the no-config default (full enum retained).allowed-eventsshould constrain the generated PR review tool schema #54331