Skip to content

Extract shared LogAndWrapCollaboratorPermission helper to eliminate parse/log/wrap duplication#4412

Merged
lpcox merged 2 commits intomainfrom
copilot/fix-duplicate-code-collaborator-permission
Apr 23, 2026
Merged

Extract shared LogAndWrapCollaboratorPermission helper to eliminate parse/log/wrap duplication#4412
lpcox merged 2 commits intomainfrom
copilot/fix-duplicate-code-collaborator-permission

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented Apr 23, 2026

guardBackendCaller.callCollaboratorPermission (internal/server/unified.go) and restBackendCaller.CallTool (internal/proxy/proxy.go) contained near-identical ~15-line blocks: JSON-unmarshal the GitHub collaborator permission response, log the result, and wrap in MCP text format. The two copies had already drifted — proxy.go was missing owner/repo/username context in its log messages.

Changes

  • internal/mcp/collaborator_permission.go — new shared helper:

    func LogAndWrapCollaboratorPermission(
        body []byte, owner, repo, username string,
        statusCode int,
        logPrintf func(format string, args ...interface{}),
    ) interface{}

    Callers pass their own logger's Printf method so log lines appear under the correct namespace. Lives in internal/mcp since both packages already import it.

  • internal/server/unified.go — replaces the 11-line block with a single call to the helper.

  • internal/proxy/proxy.go — replaces the 13-line block with the helper call; also fixes the log drift by re-extracting owner/repo/username from argsMap at the call site, making both call paths produce identical, full-context log messages.

  • internal/mcp/collaborator_permission_test.go — unit tests covering the permission-present, permission-missing, and JSON-parse-failure paths.

Warning

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
    • Triggering command: /tmp/go-build2157326120/b513/launcher.test /tmp/go-build2157326120/b513/launcher.test -test.testlogfile=/tmp/go-build2157326120/b513/testlog.txt -test.paniconexit0 -test.timeout=10m0s /tmp/go-build2157326120/b491/vet.cfg g_.a -I x_amd64/vet --gdwarf-5 resolver/unix -o x_amd64/vet -W .cfg 2622129/b314/ x_amd64/vet . --gdwarf2 --64 x_amd64/vet (dns block)
    • Triggering command: /tmp/go-build2116879286/b509/launcher.test /tmp/go-build2116879286/b509/launcher.test -test.testlogfile=/tmp/go-build2116879286/b509/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true 7f87 f87 e-handler -errorsas -ifaceassert -nilfunc e-handler --ve�� 7326120/b537/_pkg_.a -tests (dns block)
  • invalid-host-that-does-not-exist-12345.com
    • Triggering command: /tmp/go-build2157326120/b495/config.test /tmp/go-build2157326120/b495/config.test -test.testlogfile=/tmp/go-build2157326120/b495/testlog.txt -test.paniconexit0 -test.timeout=10m0s /tmp/go-build2157326120/b365/vet.cfg uf@v1.36.11/runtime/protoimpl/impl.go uf@v1.36.11/runtime/protoimpl/version.go x_amd64/vet --gdwarf-5 --64 -o x_amd64/vet -I g_.a /libexec/gcc/x86-ifaceassert x_amd64/vet s-through=-lgcc gzip s-through=-lpthr-bool x_amd64/vet (dns block)
    • Triggering command: /tmp/go-build2116879286/b491/config.test /tmp/go-build2116879286/b491/config.test -test.testlogfile=/tmp/go-build2116879286/b491/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true ry=1 -buildtags by/aeff6c0ff32d25ff2d4a63a2dff5fjson 78cd82967f007e0f14b68a622be2cb46c09/log.json -ifaceassert -nilfunc /opt/hostedtoolc/var/run/docker/runtime-runc/moby ae43�� -bool b2c23eb104bb42d0--log-format ache/go/1.25.9/xjson b2c23eb104bb42d0/usr/bin/containerd-shim-runc-v2 (dns block)
  • nonexistent.local
    • Triggering command: /tmp/go-build2157326120/b513/launcher.test /tmp/go-build2157326120/b513/launcher.test -test.testlogfile=/tmp/go-build2157326120/b513/testlog.txt -test.paniconexit0 -test.timeout=10m0s /tmp/go-build2157326120/b491/vet.cfg g_.a -I x_amd64/vet --gdwarf-5 resolver/unix -o x_amd64/vet -W .cfg 2622129/b314/ x_amd64/vet . --gdwarf2 --64 x_amd64/vet (dns block)
    • Triggering command: /tmp/go-build2116879286/b509/launcher.test /tmp/go-build2116879286/b509/launcher.test -test.testlogfile=/tmp/go-build2116879286/b509/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true 7f87 f87 e-handler -errorsas -ifaceassert -nilfunc e-handler --ve�� 7326120/b537/_pkg_.a -tests (dns block)
  • slow.example.com
    • Triggering command: /tmp/go-build2157326120/b513/launcher.test /tmp/go-build2157326120/b513/launcher.test -test.testlogfile=/tmp/go-build2157326120/b513/testlog.txt -test.paniconexit0 -test.timeout=10m0s /tmp/go-build2157326120/b491/vet.cfg g_.a -I x_amd64/vet --gdwarf-5 resolver/unix -o x_amd64/vet -W .cfg 2622129/b314/ x_amd64/vet . --gdwarf2 --64 x_amd64/vet (dns block)
    • Triggering command: /tmp/go-build2116879286/b509/launcher.test /tmp/go-build2116879286/b509/launcher.test -test.testlogfile=/tmp/go-build2116879286/b509/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true 7f87 f87 e-handler -errorsas -ifaceassert -nilfunc e-handler --ve�� 7326120/b537/_pkg_.a -tests (dns block)
  • this-host-does-not-exist-12345.com
    • Triggering command: /tmp/go-build2157326120/b522/mcp.test /tmp/go-build2157326120/b522/mcp.test -test.testlogfile=/tmp/go-build2157326120/b522/testlog.txt -test.paniconexit0 -test.timeout=10m0s kraa�� .cfg om/itchyny/gojq@v0.12.19/code.go x_amd64/vet � Warning: golan/usr/libexec/docker/docker-init /internal/timese--version -o x_amd64/vet .cfg�� 2622129/b466/_pkg_.a -trimpath x_amd64/vet -I /tmp/go-build284--version -I x_amd64/vet (dns block)
    • Triggering command: /tmp/go-build2116879286/b518/mcp.test /tmp/go-build2116879286/b518/mcp.test -test.testlogfile=/tmp/go-build2116879286/b518/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true 5db5e1bed6d3ad40/run/containerd/io.containerd.runtime.v2.task/moby/d2542b7f681d36e564fd87be0d7ce/usr/libexec/docker/cli-plugins/docker-buildx -tests by/2727682ed305djson g_.a elemetry.io/otel-atomic x_amd64/vet ker/cli-plugins/-buildtags b798�� XSX2/le4Yovkc17w-errorsas -test.timeout=10-ifaceassert by/f3249cdcb5e13-nilfunc .cfg /tmp/go-build284/usr/bin/runc x_amd64/compile docker (dns block)

If you need me to access, download, or install something from one of these locations, you can either:

Copilot AI changed the title [WIP] Fix duplicate code in collaborator permission parse/log/wrap block Extract shared LogAndWrapCollaboratorPermission helper to eliminate parse/log/wrap duplication Apr 23, 2026
Copilot AI requested a review from lpcox April 23, 2026 17:41
@lpcox lpcox marked this pull request as ready for review April 23, 2026 17:52
Copilot AI review requested due to automatic review settings April 23, 2026 17:52
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR removes duplicated “parse JSON → log permission → wrap as MCP text response” logic for the get_collaborator_permission synthetic tool by extracting it into a shared helper in internal/mcp, and updates both the unified server and proxy call paths to use it (including restoring full owner/repo/username context in proxy logging).

Changes:

  • Added mcp.LogAndWrapCollaboratorPermission helper to centralize collaborator-permission parsing/logging and MCP wrapping.
  • Replaced the inlined logging/wrapping blocks in internal/server/unified.go and internal/proxy/proxy.go with a single helper call.
  • Added unit tests covering permission present/missing and JSON-parse-failure log paths.
Show a summary per file
File Description
internal/mcp/collaborator_permission.go Introduces shared helper to log collaborator permission (with full context) and return an MCP text response.
internal/server/unified.go Uses the new helper in guardBackendCaller.callCollaboratorPermission instead of inlined parse/log/wrap logic.
internal/proxy/proxy.go Uses the new helper in restBackendCaller.CallTool and ensures owner/repo/username context is included in logs.
internal/mcp/collaborator_permission_test.go Adds unit coverage for helper behavior across success/missing-field/parse-failure cases.

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: 0

@lpcox lpcox merged commit f5df252 into main Apr 23, 2026
26 of 27 checks passed
@lpcox lpcox deleted the copilot/fix-duplicate-code-collaborator-permission branch April 23, 2026 22:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[duplicate-code] Duplicate Code Pattern: Collaborator Permission Parse/Log/Wrap Block

3 participants