Conversation
Agent-Logs-Url: https://github.com/github/gh-aw-mcpg/sessions/c177038a-3a8b-46c0-b7e6-c4dd471700cf Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR reduces internal/ micro-package/file sprawl by relocating shared helpers into common utility packages and consolidating single-purpose code into existing files, keeping behavior consistent while improving reuse and discoverability.
Changes:
- Moved version-string composition into
internal/versionviaversion.BuildVersionString(...)and updatedmain+ tests. - Centralized transient HTTP status retry classification into
internal/httputil.IsTransientHTTPErrorand updated config schema-fetch retry logic + tests. - Removed
internal/timeutilby movingFormatDurationtointernal/strutil, updated logger call sites, and collapsed micro-files inauth/oidc/loggerhelpers.
Show a summary per file
| File | Description |
|---|---|
| main.go | Switches version string assembly to version.BuildVersionString(...); removes local helper. |
| main_test.go | Updates tests to call version.BuildVersionString(...). |
| internal/version/version.go | Adds BuildVersionString implementation alongside existing version storage helpers. |
| internal/httputil/httputil.go | Adds shared IsTransientHTTPError helper. |
| internal/httputil/transient_http_error_test.go | Moves/updates tests to validate IsTransientHTTPError. |
| internal/config/validation_schema.go | Uses httputil.IsTransientHTTPError in schema fetch retry logic. |
| internal/strutil/format_duration.go | Moves duration formatting into strutil. |
| internal/strutil/format_duration_test.go | Updates duration formatting tests to strutil package. |
| internal/logger/logger.go | Updates duration formatting call sites to strutil.FormatDuration. |
| internal/logger/rpc_helpers.go | Inlines map key extraction and removes now-unneeded private helper. |
| internal/logger/rpc_helpers_test.go | Removes unit test for deleted private helper. |
| internal/auth/header.go | Inlines API key generator into header.go (and keeps logging local). |
| internal/auth/apikey.go | Removes micro-file now merged into header.go. |
| internal/oidc/provider.go | Inlines ErrMissingOIDCEnvVar into provider implementation. |
| internal/oidc/errors.go | Removes micro-file now merged into provider.go. |
| CONTRIBUTING.md | Updates package-structure docs for strutil expansion and timeutil removal. |
| AGENTS.md | Updates quick project-structure reference to reflect utility consolidation. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 17/17 changed files
- Comments generated: 1
| - **`internal/oidc/`** - OIDC authentication for HTTP MCP backends | ||
| - **`internal/proxy/`** - HTTP forward proxy applying DIFC filtering to `gh` CLI and REST/GraphQL requests | ||
| - **`internal/server/`** - HTTP server with routed and unified modes | ||
| - **`internal/strutil/`** - String utility helpers (deduplication, trimming) | ||
| - **`internal/strutil/`** - String and formatting utility helpers (deduplication, trimming, duration formatting) | ||
| - **`internal/syncutil/`** - Concurrency utility helpers (get-or-create pattern) | ||
| - **`internal/sys/`** - System utilities | ||
| - **`internal/testutil/`** - Test utilities and helpers |
There was a problem hiding this comment.
This update removes internal/timeutil from the “Key Directories” list, but the earlier “Project Structure” tree in the same document still lists internal/timeutil/ and describes internal/strutil/ only as string helpers. Please update that tree block as well so the structure documentation is consistent with the refactor.
|
@copilot address the review feedback #4117 (review) |
Agent-Logs-Url: https://github.com/github/gh-aw-mcpg/sessions/743edb2f-b58a-46e5-a74f-5ae7cf539557 Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
Addressed in d6c0c9a. I updated the CONTRIBUTING.md Project Structure tree to remove |
Semantic clustering flagged utility outliers and micro-file/package sprawl across
internal/. This PR consolidates shared helpers into appropriate common packages and removes single-purpose file/package fragmentation while preserving existing behavior.Version composition moved to
internal/versionbuildVersionStringfrommain.gointoversion.BuildVersionString(mainVersion, gitCommit, buildDate).mainnow delegates version-string assembly tointernal/version, making logic package-local and testable outsidemain.Transient HTTP classification centralized in
httputilisTransientHTTPErrorto sharedhttputil.IsTransientHTTPError.internal/configtointernal/httputil.Removed
timeutilmicro-packageFormatDurationfrominternal/timeutiltointernal/strutil.strutil.FormatDuration.internal/timeutilfiles.Collapsed micro-files in
authandoidcGenerateRandomAPIKey(and logger) intointernal/auth/header.go; removedinternal/auth/apikey.go.ErrMissingOIDCEnvVarintointernal/oidc/provider.go; removedinternal/oidc/errors.go.Inlined single-use logger helper
getMapKeysfrominternal/logger/rpc_helpers.go.Docs alignment
internal/timeutiland expandedinternal/strutilrole.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/tmp/go-build3787159813/b514/launcher.test /tmp/go-build3787159813/b514/launcher.test -test.testlogfile=/tmp/go-build3787159813/b514/testlog.txt -test.paniconexit0 -test.timeout=10m0s /tmp/go-build3787159813/b423/vet.cfg rotocol/go-sdk@v1.5.0/jsonrpc/jsonrpc.go =0 x_amd64/vet --gdwarf-5 esource/v1 -o x_amd64/vet --de�� _.a --debug-prefix-m-ifaceassert x_amd64/vet -I go-sdk/mcp -I x_amd64/vet(dns block)/tmp/go-build2056270914/b513/launcher.test /tmp/go-build2056270914/b513/launcher.test -test.testlogfile=/tmp/go-build2056270914/b513/testlog.txt -test.paniconexit0 -test.timeout=10m0s know�� known-linux-gnu/lib/rustlib/x86_/home/REDACTED/work/gh-aw-mcpg/gh-aw-mcpg/guards/github-guard/rustgit known-linux-gnu/lib/rustlib/x86_/home/REDACTED/work/gh-aw-mcpg/gh-aw-mcpg/guards/github-guard/rustpush x_amd64/compile known-linux-gnu/git known-linux-gnu/ls-files known-linux-gnu/--exclude-standard x_amd64/compile know�� known-linux-gnu/lib/rustlib/x86_64-REDACTED-linux-gnu/lib/libminiz_oxide-2b6a8d2f6e1dc71b.rlib known-linux-gnu/lib/rustlib/x86_64-REDACTED-linux-gnu/lib/libadler2-39ffdbc27c978ccc.rlib /opt/containerd/bin/bash /run/containerd/git 7213e0dede2210feconfig json 7213e0dede2210fed31/init.pid(dns block)invalid-host-that-does-not-exist-12345.com/tmp/go-build3787159813/b496/config.test /tmp/go-build3787159813/b496/config.test -test.testlogfile=/tmp/go-build3787159813/b496/testlog.txt -test.paniconexit0 -test.timeout=10m0s /tmp/go-build3787159813/b317/vet.cfg @v1.1.3/cpu/cpu.-errorsas 89155655/b151//_-ifaceassert x_amd64/vet --gdwarf-5 --64 155655/b151/ x_amd64/vet -w ache/go/1.25.8/x-errorsas dHAhgBmen x_amd64/vet OUTPUT -d ut-3746032072.c x_amd64/vet(dns block)/tmp/go-build1806864329/b491/config.test /tmp/go-build1806864329/b491/config.test -test.testlogfile=/tmp/go-build1806864329/b491/testlog.txt -test.paniconexit0 -test.timeout=10m0s bug/deps/serde_derive-bdc7cd22a58a5141.serde_derive.12123747d8da05ed-cgu.04.rcgu-importcfg bug/deps/serde_derive-bdc7cd22a58a5141.serde_derive.12123747d8da05ed-cgu.05.rcgu/tmp/go-build1806864329/b237/importcfg bug/deps/serde_derive-bdc7cd22a58a5141.serde_derive.12123747d8da05ed-cgu.06.rcgu-pack bug/deps/serde_derive-bdc7cd22a58a5141.serde_derive.12123747d8da05ed-cgu.07.rcgu/home/REDACTED/work/gh-aw-mcpg/gh-aw-mcpg/internal/difc/agent.go bug/deps/serde_derive-bdc7cd22a58a5141.serde_derive.12123747d8da05ed-cgu.08.rcgu/home/REDACTED/work/gh-aw-mcpg/gh-aw-mcpg/internal/difc/capabilities.go bug/deps/serde_derive-bdc7cd22a58a5141.serde_derive.12123747d8da05ed-cgu.09.rcgu.o bug/deps/serde_derive-bdc7cd22a58a5141.serde_derive.12123747d8da05ed-cgu.10.rcgu.o bug/deps/serde_derive-bdc7cd22a58a5141.serde_derive.12123747d8da05ed-cgu.11.rcgu.o bug/�� bug/deps/serde_derive-bdc7cd22a58a5141.serde_derive.12123747d8da05ed-cgu.13.rcgu.o bug/deps/serde_derive-bdc7cd22a58a5141.serde_derive.12123747d8da05ed-cgu.14.rcgu.o -guard/target/de--build-id ntime.v2.task/mo/opt/copilot-runtime/copilot-developer-action-main/dist/ripgrep/bin/linux-x64/rg-trimpath /tmp/go-build389--files lib/rustlib/x86_--hidden lib/rustlib/x86_--glob(dns block)nonexistent.local/tmp/go-build3787159813/b514/launcher.test /tmp/go-build3787159813/b514/launcher.test -test.testlogfile=/tmp/go-build3787159813/b514/testlog.txt -test.paniconexit0 -test.timeout=10m0s /tmp/go-build3787159813/b423/vet.cfg rotocol/go-sdk@v1.5.0/jsonrpc/jsonrpc.go =0 x_amd64/vet --gdwarf-5 esource/v1 -o x_amd64/vet --de�� _.a --debug-prefix-m-ifaceassert x_amd64/vet -I go-sdk/mcp -I x_amd64/vet(dns block)/tmp/go-build2056270914/b513/launcher.test /tmp/go-build2056270914/b513/launcher.test -test.testlogfile=/tmp/go-build2056270914/b513/testlog.txt -test.paniconexit0 -test.timeout=10m0s know�� known-linux-gnu/lib/rustlib/x86_/home/REDACTED/work/gh-aw-mcpg/gh-aw-mcpg/guards/github-guard/rustgit known-linux-gnu/lib/rustlib/x86_/home/REDACTED/work/gh-aw-mcpg/gh-aw-mcpg/guards/github-guard/rustpush x_amd64/compile known-linux-gnu/git known-linux-gnu/ls-files known-linux-gnu/--exclude-standard x_amd64/compile know�� known-linux-gnu/lib/rustlib/x86_64-REDACTED-linux-gnu/lib/libminiz_oxide-2b6a8d2f6e1dc71b.rlib known-linux-gnu/lib/rustlib/x86_64-REDACTED-linux-gnu/lib/libadler2-39ffdbc27c978ccc.rlib /opt/containerd/bin/bash /run/containerd/git 7213e0dede2210feconfig json 7213e0dede2210fed31/init.pid(dns block)slow.example.com/tmp/go-build3787159813/b514/launcher.test /tmp/go-build3787159813/b514/launcher.test -test.testlogfile=/tmp/go-build3787159813/b514/testlog.txt -test.paniconexit0 -test.timeout=10m0s /tmp/go-build3787159813/b423/vet.cfg rotocol/go-sdk@v1.5.0/jsonrpc/jsonrpc.go =0 x_amd64/vet --gdwarf-5 esource/v1 -o x_amd64/vet --de�� _.a --debug-prefix-m-ifaceassert x_amd64/vet -I go-sdk/mcp -I x_amd64/vet(dns block)/tmp/go-build2056270914/b513/launcher.test /tmp/go-build2056270914/b513/launcher.test -test.testlogfile=/tmp/go-build2056270914/b513/testlog.txt -test.paniconexit0 -test.timeout=10m0s know�� known-linux-gnu/lib/rustlib/x86_/home/REDACTED/work/gh-aw-mcpg/gh-aw-mcpg/guards/github-guard/rustgit known-linux-gnu/lib/rustlib/x86_/home/REDACTED/work/gh-aw-mcpg/gh-aw-mcpg/guards/github-guard/rustpush x_amd64/compile known-linux-gnu/git known-linux-gnu/ls-files known-linux-gnu/--exclude-standard x_amd64/compile know�� known-linux-gnu/lib/rustlib/x86_64-REDACTED-linux-gnu/lib/libminiz_oxide-2b6a8d2f6e1dc71b.rlib known-linux-gnu/lib/rustlib/x86_64-REDACTED-linux-gnu/lib/libadler2-39ffdbc27c978ccc.rlib /opt/containerd/bin/bash /run/containerd/git 7213e0dede2210feconfig json 7213e0dede2210fed31/init.pid(dns block)this-host-does-not-exist-12345.com/tmp/go-build3787159813/b523/mcp.test /tmp/go-build3787159813/b523/mcp.test -test.testlogfile=/tmp/go-build3787159813/b523/testlog.txt -test.paniconexit0 -test.timeout=10m0s -W cfg /tmp/go-build389-ifaceassert x_amd64/vet _amd64.s g/protobuf/proto--version --64 x_amd64/vet cfg 155655/b281/_pkg_.a -fPIC x_amd64/vet -pthread .io/otel/exporte/usr/bin/runc -fmessage-length--version x_amd64/vet(dns block)/tmp/go-build2056270914/b522/mcp.test /tmp/go-build2056270914/b522/mcp.test -test.testlogfile=/tmp/go-build2056270914/b522/testlog.txt -test.paniconexit0 -test.timeout=10m0s go1.25.8 -c=4 -nolocalimports -importcfg /tmp/go-build3333051407/b001/importcfg -pack /home/REDACTED/work/gh-aw-mcpg/gh-aw-mcpg/main.go /home/REDACTED/work/gh-aw-mcpg/gh-aw-mcpg/version.go ive.�� ive.12123747d8da05ed-cgu.02.rcgugo1.25.8 ive.12123747d8da05ed-cgu.03.rcgu-c=4 ive.12123747d8da05ed-cgu.04.rcgu-nolocalimports ive.12123747d8da/opt/hostedtoolcache/go/1.25.8/x64/pkg/tool/linux_amd64/link ive.12123747d8da-V=full ive.12123747d8da05ed-cgu.07.rcgu-bool ive.12123747d8da05ed-cgu.08.rcgu-buildtags(dns block)If you need me to access, download, or install something from one of these locations, you can either: