Formal spec alignment for GitHub MCP access-control conformance docs and predicate mapping#47633
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
🤖 PR Triage
Score breakdown: Impact 20 + Urgency 15 + Quality 15 Batch: Rationale: Draft PR updating formal spec for GitHub MCP access-control compliance. Only 2 files (a Go test + README). No CI data. Groups with #47637 for a doc/spec batch review.
|
|
Great work on keeping the formal spec artifacts and conformance suite synchronized! 🎯 This correction to the predicate label mapping ( Here's one thing that would strengthen this PR:
If you'd like to enhance this, you can assign this prompt to your coding agent:
|
🤖 PR Triage — §30057272009
Score breakdown: Impact 20 + Urgency 5 + Quality 3 Rationale: DRAFT PR. Formal spec alignment docs only. No CI, no reviews. Defer until undrafted and CI passes. Next action: Undraft, ensure CI passes, then consider merging alongside #47637.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Test Quality Sentinel completed test quality analysis. No new or modified test functions detected. The PR only updates a comment ('P1_RepoMatch' → 'P2_RepoMatch') in an existing test. Test Quality Sentinel skipped. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
There was a problem hiding this comment.
Pull request overview
Aligns formal access-control documentation and predicate annotations with the executable conformance suite.
Changes:
- Updates usage instructions for running formal tests.
- Corrects the repository predicate label from
P1toP2.
Show a summary per file
| File | Description |
|---|---|
specs/github-mcp-access-control-compliance/README.md |
Documents formal suite entrypoints and test-file contract. |
pkg/workflow/github_mcp_access_control_formal_test.go |
Corrects the omitted-repository predicate annotation. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 0
- Review effort level: Medium
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR does not have the 'implementation' label and has ≤100 new lines of code in business logic directories (default_business_additions=1). |
There was a problem hiding this comment.
No blocking issues. The predicate label correction (P1_RepoMatch to P2_RepoMatch) is accurate per the documented ordering (P1=ToolAllowed, P2=RepoMatch), and the README simplification is clean.
🔎 Code quality review by PR Code Quality Reviewer · sonnet46 · 13.2 AIC · ⌖ 4.46 AIC · ⊞ 5.7K
Comment /review to run again
There was a problem hiding this comment.
Review
Changes look correct.
- Predicate fix:
P1_RepoMatch→P2_RepoMatchis accurate given the defined ordering (P1=ToolAllowed, P2=RepoMatch). - README simplification: Replacing the explicit long regex with
TestFormal_prefix is cleaner and future-proof as new tests are added.
Minor note: the removal of the TestValidateGitHubGuardPolicy run instructions from the README reduces discoverability for those §11.1.x tests, but this is a deliberate scope decision and not blocking.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 18.1 AIC · ⌖ 4.49 AIC · ⊞ 5K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /grill-with-docs — this is a clean, focused documentation and comment-fix PR. Approving.
📋 Summary
What changed
- Predicate label fix (
P1_RepoMatch→P2_RepoMatchinTestFormal_OmittedReposAllowAll): Correctly aligns with the formal guard ordering defined in the spec (P1=ToolAllowed,P2=RepoMatch). Verified against the spec README lines 23–43. - README simplification: Replaced a fragile exhaustive test-name list with the simpler
TestFormal_prefix pattern, which is less likely to drift as new tests are added.
Positive Highlights
- ✅ Predicate fix is accurate and traceable to the spec definition
- ✅ Simplified
go test -run "TestFormal_"pattern is less fragile than the old exhaustive named list - ✅ Numbered steps in the Usage section improve readability
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 26.8 AIC · ⌖ 4.55 AIC · ⊞ 6.7K
Comment /matt to run again
|
@copilot run pr-finishser skill |
|
🎉 This pull request is included in a new release. Release: |
This updates the formal-spec artifacts for GitHub MCP access-control compliance so the documented model, coverage map, and execution instructions stay synchronized with the executable conformance suite. It also corrects a predicate label mismatch in the formal test commentary to match the defined
P1–P6ordering.Spec usage contract alignment
specs/github-mcp-access-control-compliance/README.md.Predicate mapping consistency
TestFormal_OmittedReposAllowAllfromP1_RepoMatchtoP2_RepoMatchinpkg/workflow/github_mcp_access_control_formal_test.go, matching the formal guard ordering (P1=ToolAllowed,P2=RepoMatch).Example of the corrected predicate annotation: