feat: Maintainer reaction endorsement for integrity promotion/demotion#3666
Merged
feat: Maintainer reaction endorsement for integrity promotion/demotion#3666
Conversation
- Add endorsement_reactions, disapproval_reactions, disapproval_integrity, endorser_min_integrity fields to PolicyContext in Rust guard - Implement has_maintainer_reaction_with_callback() core evaluation function with gateway-mode graceful degradation (log once, skip safely) - Add has_maintainer_endorsement() and has_maintainer_disapproval() functions - Add apply_endorsement_promotion() and apply_disapproval_demotion() functions - Wire endorsement (step 3) and disapproval (step 4, always wins) into issue_integrity() and pr_integrity() after approval-labels promotion - Add cap_integrity() helper for disapproval demotion (min of current and cap) - Add collaborator permission caching in backend.rs (keyed owner/repo:user) - Update AllowOnlyPolicy in lib.rs to deserialize new fields - Update Go config AllowOnlyPolicy and NormalizedGuardPolicy structs - Update JSON marshal/unmarshal with new field support - Update NormalizeGuardPolicy with validation for reaction fields - Update wasm.go buildStrictLabelAgentPayload to allow new allow-only keys - Add comprehensive unit tests (Rust and Go) for all new functionality Agent-Logs-Url: https://github.com/github/gh-aw-mcpg/sessions/1cd99fee-78d0-43bd-bc83-1d39e17d9de3 Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
- Use separate AtomicBool flags for endorsement vs disapproval gateway warnings so each reaction kind logs its own warning at most once per process lifetime - Add warning log for unrecognised integrity level values in integrity_level_rank() (defaults to 'approved' per spec, now with explicit warning) - Document cache key username lowercasing behavior in get_collaborator_permission_with_callback Agent-Logs-Url: https://github.com/github/gh-aw-mcpg/sessions/1cd99fee-78d0-43bd-bc83-1d39e17d9de3 Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
Copilot
AI
changed the title
[WIP] Add maintainer reaction endorsement for integrity promotion
feat: Maintainer reaction endorsement for integrity promotion/demotion
Apr 12, 2026
Contributor
There was a problem hiding this comment.
Pull request overview
Adds reaction-based maintainer endorsement/disapproval signals to the GitHub guard’s integrity evaluation, with corresponding policy/config plumbing and validation across the Go gateway and Rust WASM guard.
Changes:
- Extend allow-only policy schema to support endorsement/disapproval reaction lists and integrity thresholds/caps.
- Implement reaction evaluation in the Rust guard (promotion to approved; demotion cap with disapproval override) with gateway-mode degradation behavior.
- Add Go-side normalization/validation and expand unit tests for the new policy fields and behavior.
Show a summary per file
| File | Description |
|---|---|
| internal/guard/wasm.go | Allows/validates the new allow-only keys in the strict payload builder. |
| internal/guard/wasm_test.go | Adds payload-shape tests for new reaction + integrity config fields. |
| internal/config/guard_policy.go | Extends policy structs + JSON (un)marshal; normalizes reaction lists and validates new integrity fields. |
| internal/config/guard_policy_test.go | Adds normalization + round-trip tests for the new allow-only fields. |
| guards/github-guard/rust-guard/src/lib.rs | Wires new allow-only fields into runtime PolicyContext. |
| guards/github-guard/rust-guard/src/labels/mod.rs | Adds Rust tests covering reaction-related integrity edge cases/degradation. |
| guards/github-guard/rust-guard/src/labels/helpers.rs | Implements reaction-based endorsement/disapproval evaluation and integrity promotion/demotion helpers. |
| guards/github-guard/rust-guard/src/labels/backend.rs | Adds caching for collaborator permission lookups to reduce repeated backend calls. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 8/8 changed files
- Comments generated: 3
Comment on lines
+27
to
+33
| /// Cache for collaborator permission lookups keyed by "owner/repo:username". | ||
| /// Caches the raw permission string so it can be reused across multiple items | ||
| /// that share the same reactor within a single gateway request. | ||
| fn collaborator_permission_cache() -> &'static Mutex<HashMap<String, Option<String>>> { | ||
| static CACHE: OnceLock<Mutex<HashMap<String, Option<String>>>> = OnceLock::new(); | ||
| CACHE.get_or_init(|| Mutex::new(HashMap::new())) | ||
| } |
There was a problem hiding this comment.
The collaborator-permission cache is implemented as a process-wide static (OnceLock + Mutex) and is never cleared/evicted, but the comment states it is per-request. This can lead to unbounded growth (unique user/repo combos) and stale permission decisions affecting integrity promotion/demotion; consider scoping this cache to a single gateway request (or adding TTL/LRU + explicit reset at request boundaries) and updating the comment accordingly.
Comment on lines
+490
to
+505
| // Cache key uses lowercase username because GitHub usernames are case-insensitive. | ||
| // The original case-sensitive username is preserved in the returned CollaboratorPermission | ||
| // struct (via `username.to_string()`) so callers see the canonical display form. | ||
| let cache_key = format!("{}/{}:{}", owner, repo, username.to_ascii_lowercase()); | ||
|
|
||
| // Return cached permission if available. | ||
| if let Some(cached) = get_cached_collaborator_permission(&cache_key) { | ||
| crate::log_debug(&format!( | ||
| "get_collaborator_permission: cache hit for {}/{} user {} → permission={:?}", | ||
| owner, repo, username, cached | ||
| )); | ||
| return cached.map(|permission| CollaboratorPermission { | ||
| permission: Some(permission), | ||
| login: Some(username.to_string()), | ||
| }); | ||
| } |
There was a problem hiding this comment.
The cache key only normalizes username casing; owner/repo are also case-insensitive on GitHub, so differing case in inputs will reduce cache hits and increase cache size. Consider normalizing owner/repo to a canonical form (e.g., lowercasing) when building the cache key.
Comment on lines
+370
to
+384
| /// Convert an integrity level name to its rank (0 = unknown, 1 = none, 2 = reader, 3 = writer, 4 = merged). | ||
| fn integrity_level_rank(level: &str) -> u8 { | ||
| match level.trim().to_ascii_lowercase().as_str() { | ||
| "none" => 1, | ||
| "unapproved" => 2, | ||
| "approved" => 3, | ||
| "merged" => 4, | ||
| other => { | ||
| crate::log_warn(&format!( | ||
| "integrity_level_rank: unrecognised level {:?}, defaulting to 'approved'", | ||
| other | ||
| )); | ||
| 3 // unrecognised → safe default is "approved" (matches endorser_min_integrity default) | ||
| } | ||
| } |
There was a problem hiding this comment.
The doc comment for integrity_level_rank says rank 0 represents "unknown", but the implementation never returns 0 and instead defaults unrecognized levels to rank 3 (approved). Update the comment to match the actual behavior (or adjust the function) to avoid misleading future changes.
…ring - Update collaborator permission cache comment to accurately describe it as process-wide static (not per-request). - Normalize owner/repo to lowercase in cache keys since GitHub treats them as case-insensitive, improving cache hit rate. - Fix integrity_level_rank docstring: remove incorrect '0 = unknown' and 'reader/writer' terminology; document actual return values. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The function was defined in helpers.rs tests but never called (the equivalent helper in mod.rs tests is the one actually used). Fixes rust-guard-test CI failure: -D dead-code triggered by RUSTFLAGS. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…tests - Add critical test: disapproval overrides endorsement on same item (helpers.rs) verifying the precedence rule via mock callbacks - Add tests for disapproval with admin/read permission thresholds - Fix misleading Go comments claiming defaults are set by normalization (defaults are applied in Rust, not Go) - Add Go tests: empty disapproval-reactions entry rejected, disapproval-reactions deduplication Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The test_has_maintainer_reaction_backend_error_skips test was flaky because it used login 'alice' which could be cached by prior tests using admin_permission_callback. The global permission cache returned the cached 'admin' permission instead of invoking error_callback. Use 'error-test-user' to guarantee a cache miss. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This was referenced Apr 12, 2026
4 tasks
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.
Adds two new integrity mechanisms to the AllowOnly guard: maintainer 👍/❤️ reactions can promote an item to
approvedintegrity, and maintainer 👎/😕 reactions can cap it at a configured level (defaultnone). Disapproval always overrides endorsement.Config
{ "allow-only": { "min-integrity": "approved", "endorsement-reactions": ["THUMBS_UP", "HEART"], "disapproval-reactions": ["THUMBS_DOWN", "CONFUSED"], "disapproval-integrity": "none", "endorser-min-integrity": "approved" } }endorsement-reactions/disapproval-reactions:ReactionContentenum values. Empty = disabled.disapproval-integrity: integrity cap when disapproval detected. Defaultnone.endorser-min-integrity: minimum collaborator permission for a reactor to count. Defaultapproved(write/maintain/admin).Precedence (new steps 3 & 4)
blocked_users> author_association > trusted_users > merged > approval_labels > endorsement > disapprovalRust guard (
helpers.rs,lib.rs)PolicyContextfieldshas_maintainer_reaction_with_callback(): extractsreactions.nodes[]{user.login, content}(GraphQL proxy shape), checks up to 20 reactions, resolves reactor integrity viaget_collaborator_permissionapply_endorsement_promotion()/apply_disapproval_demotion()wired as steps 3/4 inissue_integrity()andpr_integrity()cap_integrity(): min-of-two for demotion (inverse ofmax_integrity)AllowOnlyPolicydeserialization extended for the 4 new fieldsGateway-mode degradation
When
reactionsfield is present but has nonodesarray (MCP server returns counts only, not per-user data), each reaction kind logs a warning once per process lifetime and skips evaluation entirely — no promotion or demotion occurs.Backend (
backend.rs)get_collaborator_permissionkeyedowner/repo:username(case-insensitive) to bound API calls when multiple items share the same reactorGo config (
guard_policy.go,wasm.go)AllowOnlyPolicy+NormalizedGuardPolicystructs extended with 4 new fieldsNormalizeGuardPolicy: uppercases reaction content values, deduplicates, validates integrity level stringsbuildStrictLabelAgentPayload: allows and validates the 4 newallow-onlykeysWarning
Firewall rules blocked me from connecting to one or more addresses (expand for details)
I tried to connect to the following addresses, but was blocked by firewall rules:
example.com/tmp/go-build2564705994/b514/launcher.test /tmp/go-build2564705994/b514/launcher.test -test.testlogfile=/tmp/go-build2564705994/b514/testlog.txt -test.paniconexit0 -test.timeout=10m0s /tmp/go-build2564705994/b422/vet.cfg(dns block)invalid-host-that-does-not-exist-12345.com/tmp/go-build2193583002/b001/config.test /tmp/go-build2193583002/b001/config.test -test.testlogfile=/tmp/go-build2193583002/b001/testlog.txt -test.paniconexit0 -test.timeout=10m0s aw-m�� go aw-mcpg/guards/g--64 x_amd64/asm aw-mcpg/guards/g/opt/hostedtoolcache/go/1.25.8/x64/pkg/tool/linux_amd64/vet aw-mcpg/guards/g-unsafeptr=false aw-mcpg/guards/g-unreachable=false x_amd64/asm aw-m�� g_.a Lj2wtMmGc x_amd64/compile k/gh-aw-mcpg/gh-/opt/hostedtoolcache/go/1.25.8/x64/pkg/tool/linux_amd64/vet(dns block)/tmp/go-build2936037720/b001/config.test /tmp/go-build2936037720/b001/config.test -test.testlogfile=/tmp/go-build2936037720/b001/testlog.txt -test.paniconexit0 -test.timeout=10m0s --uid-owner 0 -j ACCEPT /home/REDACTED/wor/usr/lib/sysstat/sadc /home/REDACTED/wor-F /home/REDACTED/wor-L x_amd64/vet go_.�� 64/src/net ache/go/1.25.8/x1 64/pkg/tool/linu/var/log/sysstat -p internal/runtime-w -lang=go1.25 64/pkg/tool/linusecurity(dns block)nonexistent.local/tmp/go-build2564705994/b514/launcher.test /tmp/go-build2564705994/b514/launcher.test -test.testlogfile=/tmp/go-build2564705994/b514/testlog.txt -test.paniconexit0 -test.timeout=10m0s /tmp/go-build2564705994/b422/vet.cfg(dns block)slow.example.com/tmp/go-build2564705994/b514/launcher.test /tmp/go-build2564705994/b514/launcher.test -test.testlogfile=/tmp/go-build2564705994/b514/testlog.txt -test.paniconexit0 -test.timeout=10m0s /tmp/go-build2564705994/b422/vet.cfg(dns block)this-host-does-not-exist-12345.com/tmp/go-build2564705994/b523/mcp.test /tmp/go-build2564705994/b523/mcp.test -test.testlogfile=/tmp/go-build2564705994/b523/testlog.txt -test.paniconexit0 -test.timeout=10m0s /tmp/go-build2564705994/b446/vet.cfg g_.a /home/REDACTED/go/pkg/mod/github.c-nolocalimports x_amd64/vet 64/src/runtime/c/opt/hostedtoolcache/go/1.25.8/x64/pkg/tool/linux_amd64/vet json x_amd64/asm x_amd64/vet --no�� 1.80.0/grpclog/c-errorsas 1.80.0/grpclog/g-ifaceassert x_amd64/vet 64/src/runtime/c/opt/hostedtoolcache/go/1.25.8/x64/pkg/tool/linux_amd64/vet k/gh-aw-mcpg/gh--atomic x_amd64/compile x_amd64/vet(dns block)If you need me to access, download, or install something from one of these locations, you can either: