refactor: extract ParseCollaboratorPermissionArgs to eliminate cross-package duplication#4497
Merged
lpcox merged 4 commits intoApr 24, 2026
Merged
Conversation
Closed
6 tasks
…ate arg-parsing logic - Add ParseCollaboratorPermissionArgs to internal/mcp/collaborator_permission.go - Add TestParseCollaboratorPermissionArgs with full coverage - Use helper in internal/server/unified.go callCollaboratorPermission - Use helper in internal/proxy/proxy.go restBackendCaller.CallTool (also eliminates post-switch argsMap re-read via function-scoped vars) Agent-Logs-Url: https://github.com/github/gh-aw-mcpg/sessions/9f7bc56f-e889-43d6-9774-665d50432ba0 Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
Copilot
AI
changed the title
[WIP] Refactor duplicate get_collaborator_permission logic
refactor: extract ParseCollaboratorPermissionArgs to eliminate cross-package duplication
Apr 24, 2026
Contributor
There was a problem hiding this comment.
Pull request overview
Refactors duplicated get_collaborator_permission argument parsing (owner/repo/username extraction + validation) into a shared helper in internal/mcp, keeping the unified server and proxy REST implementations in sync.
Changes:
- Added
mcp.ParseCollaboratorPermissionArgsto centralize parsing/validation and return partial values on error for better logging. - Updated unified server and proxy call paths to use the shared helper (and avoid re-reading args in the proxy).
- Added unit tests for the new parsing helper.
Show a summary per file
| File | Description |
|---|---|
| internal/mcp/collaborator_permission.go | Adds shared ParseCollaboratorPermissionArgs helper used by both server and proxy. |
| internal/mcp/collaborator_permission_test.go | Adds tests covering success, missing-field errors, and partial-return behavior. |
| internal/server/unified.go | Replaces inline parsing in callCollaboratorPermission with the shared helper. |
| internal/proxy/proxy.go | Uses the shared helper in restBackendCaller.CallTool and reuses parsed values for post-call logging/wrapping. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 4/4 changed files
- Comments generated: 1
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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.
The
get_collaborator_permissionarg parsing logic (owner/repo/usernameextraction + validation) was duplicated verbatim ininternal/server/unified.goandinternal/proxy/proxy.go, creating a drift risk between two separate HTTP implementations.Changes
internal/mcp/collaborator_permission.go— newParseCollaboratorPermissionArgs(argsMap map[string]interface{}) (owner, repo, username string, err error)helper; returns partial values on error so callers can include them in log messagesinternal/mcp/collaborator_permission_test.go—TestParseCollaboratorPermissionArgscovering happy path, each missing field, and partial-value-on-error behaviorinternal/server/unified.go—callCollaboratorPermissionreplaces 8-line inline extraction with the shared helperinternal/proxy/proxy.go—restBackendCaller.CallTooluses the helper via function-scopedcollabOwner/collabRepo/collabUsernamevars, also eliminating the redundant post-switchargsMapre-readWarning
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-build2075080314/b509/launcher.test /tmp/go-build2075080314/b509/launcher.test -test.testlogfile=/tmp/go-build2075080314/b509/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true p/go-build main x_amd64/compile(dns block)/tmp/go-build1743922486/b513/launcher.test /tmp/go-build1743922486/b513/launcher.test -test.testlogfile=/tmp/go-build1743922486/b513/testlog.txt -test.paniconexit0 -test.timeout=10m0s -o 5080314/b497/difc.test -trimpath x_amd64/vet -p google.golang.or-atomic -lang=go1.24 x_amd64/vet -o ry=1 -trimpath x_amd64/vet -p google.golang.or/usr/bin/runc -lang=go1.24 x_amd64/vet(dns block)invalid-host-that-does-not-exist-12345.com/tmp/go-build2075080314/b491/config.test /tmp/go-build2075080314/b491/config.test -test.testlogfile=/tmp/go-build2075080314/b491/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true 64/src/runtime/c-p o x_amd64/asm(dns block)/tmp/go-build1743922486/b495/config.test /tmp/go-build1743922486/b495/config.test -test.testlogfile=/tmp/go-build1743922486/b495/testlog.txt -test.paniconexit0 -test.timeout=10m0s -o zU9f4ahsV -trimpath ache/go/1.25.9/x64/pkg/tool/linux_amd64/vet -p go.opentelemetry/tmp/go-build1387771901/b265/vet.cfg -lang=go1.25 ortcfg -I aw-mcpg/internal/launcher/connection_pool.go aw-mcpg/internal/launcher/health_monitor.go(dns block)nonexistent.local/tmp/go-build2075080314/b509/launcher.test /tmp/go-build2075080314/b509/launcher.test -test.testlogfile=/tmp/go-build2075080314/b509/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true p/go-build main x_amd64/compile(dns block)/tmp/go-build1743922486/b513/launcher.test /tmp/go-build1743922486/b513/launcher.test -test.testlogfile=/tmp/go-build1743922486/b513/testlog.txt -test.paniconexit0 -test.timeout=10m0s -o 5080314/b497/difc.test -trimpath x_amd64/vet -p google.golang.or-atomic -lang=go1.24 x_amd64/vet -o ry=1 -trimpath x_amd64/vet -p google.golang.or/usr/bin/runc -lang=go1.24 x_amd64/vet(dns block)slow.example.com/tmp/go-build2075080314/b509/launcher.test /tmp/go-build2075080314/b509/launcher.test -test.testlogfile=/tmp/go-build2075080314/b509/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true p/go-build main x_amd64/compile(dns block)/tmp/go-build1743922486/b513/launcher.test /tmp/go-build1743922486/b513/launcher.test -test.testlogfile=/tmp/go-build1743922486/b513/testlog.txt -test.paniconexit0 -test.timeout=10m0s -o 5080314/b497/difc.test -trimpath x_amd64/vet -p google.golang.or-atomic -lang=go1.24 x_amd64/vet -o ry=1 -trimpath x_amd64/vet -p google.golang.or/usr/bin/runc -lang=go1.24 x_amd64/vet(dns block)this-host-does-not-exist-12345.com/tmp/go-build2075080314/b518/mcp.test /tmp/go-build2075080314/b518/mcp.test -test.testlogfile=/tmp/go-build2075080314/b518/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true rnal/pragma/prag-errorsas ache/go/1.25.9/x-ifaceassert 64/pkg/tool/linu-nilfunc -p syscall -lang=go1.25 64/pkg/tool/linu-buildtags go_.�� dns/dns_resolver-errorsas .cfg x_amd64/compile -p crypto/ed25519 -lang=go1.25 x_amd64/compile(dns block)/tmp/go-build1743922486/b522/mcp.test /tmp/go-build1743922486/b522/mcp.test -test.testlogfile=/tmp/go-build1743922486/b522/testlog.txt -test.paniconexit0 -test.timeout=10m0s -o /tmp/go-build2075080314/b439/_pkg_.a -trimpath x_amd64/vet -p google.golang.or--version -lang=go1.25 x_amd64/vet -ato�� submodules | head -n 10 din}} x_amd64/vet -errorsas -ifaceassert -nilfunc x_amd64/vet(dns block)/tmp/go-build3917726863/b253/mcp.test /tmp/go-build3917726863/b253/mcp.test -test.testlogfile=/tmp/go-build3917726863/b253/testlog.txt -test.paniconexit0 -test.timeout=10m0s bug/�� bug/deps/github_guard-57d41235e07a5585.3r7b32ab9hw1mmy6a1aekmmsh.1qicuz7.rcgu.o bug/deps/github_guard-57d41235e07a5585.485oswk5h4p2kq4dabhq5b2ke.1qicuz7.rcgu.o -guard/target/de--diagnostic-width=120 by/5614fb8de41e5mktemp -trimpath lib/rustlib/x86_codeql.XXXXXXXX lib/rustlib/x86_-C lib/�� -guard/target/debug/deps/rustcHdHtL6/symbols.o -guard/target/debug/deps/github_guard-57d41235e07a5585.0r6f2y9pmz8tylr32cgwnziux.1qicuz7.rcgu.o -guard/target/debug/deps/github_guard-57d41235e07a5585.0y8i0suihruczucboywd9kbz6.1qicuz7.rcgu.o -guard/target/de/bin/sh -guard/target/de-c -guard/target/deecho " release - Trigger the release workflow (usage: make release patch|minor|major)" -guard/target/debug/deps/github_guard-57d41235e07a5585.1yg4dgf4ofc88gtczrpthgg1u.1qicuz7.rcgu.o(dns block)If you need me to access, download, or install something from one of these locations, you can either: