Refactor logger persistence wrappers and document env expansion semantics by Copilot · Pull Request #10086 · github/gh-aw-mcpg

Copilot · 2026-07-25T23:13:25Z

Semantic clustering flagged low-effort cleanup opportunities: thin persistence wrappers in logger internals and potential confusion between two env-expansion paths with different failure semantics. This PR applies those targeted refactors without changing behavior.

Logger internals: remove thin writeToFile indirection
- In internal/logger/tools_logger.go, inlined writeToFile() into LogTools by calling writeJSON(tl.data, 0644) directly.
- In internal/logger/observed_url_domains_logger.go, removed writeToFile() and inlined serialization + writeJSON(..., 0600) at write sites (init + changed-domain flush path).
Env expansion docs: clarify intentional semantic split
- In internal/envutil/expand_env_args.go, added a cross-reference note to internal/config/expand.go clarifying:
  - envutil expansion passes through missing env vars unchanged
  - config expansion fails on undefined ${VAR}
Test alignment with refactor
- Updated internal/logger/tools_logger_test.go to validate the same file-write success/error paths through writeJSON(...) directly after removing writeToFile().

// Before
return tl.writeToFile()

// After
return tl.writeJSON(tl.data, 0644)

Fixes [refactor] Semantic Function Clustering Analysis — Refactoring Opportunities #10083

Copilot

Pull request overview

Refactors logger persistence paths without intended behavior changes and clarifies environment-expansion semantics.

Changes:

Inlines JSON persistence calls in logger implementations.
Updates related persistence tests.
Documents differing undefined-variable behavior.

Show a summary per file

File	Description
`internal/logger/tools_logger.go`	Removes the persistence wrapper.
`internal/logger/tools_logger_test.go`	Tests `writeJSON` directly.
`internal/logger/observed_url_domains_logger.go`	Inlines serialization and persistence.
`internal/envutil/expand_env_args.go`	Clarifies environment-expansion semantics.

Review details

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comments suppressed due to low confidence (2)

internal/logger/tools_logger_test.go:258

This direct writeJSON test still has the stale TestWriteToFile_WriteFileFails name and comment, even though writeToFile no longer exists. Rename both references to keep test failures searchable against the current symbol.

	err := tl.writeJSON(tl.data, 0644)

internal/logger/tools_logger_test.go:280

This test was switched to writeJSON, but the enclosing name and comment still reference the deleted writeToFile method. Rename them to reflect the code path now being tested.

	err := tl.writeJSON(tl.data, 0644)

Files reviewed: 4/4 changed files
Comments generated: 2
Review effort level: Medium


 		// Write the updated data to file
-		return tl.writeToFile()
+		return tl.writeJSON(tl.data, 0644)


github-actions · 2026-07-26T16:09:53Z

🔒 mcpg Read-Only Stress — gVisor

Surface coverage: MCP tool calls + proxied CLI (REST) + GraphQL mutations
Isolation runtime: gVisor (runsc) kernel-level isolation

Part	Surface	Op	Result	Expected	Status
A	MCP	reads (list_issues, list_prs, get_file, list_commits)	data returned	ALLOWED	✅
B	MCP	writes (reaction/star/issue/comment/branch/file/PR)	Error[-32602]: unknown tool (gateway allowlist)	BLOCKED	✅
C	CLI	reads (list_issues, get_file_contents)	data returned	ALLOWED	✅
D	CLI	REST writes (reaction/star/issue/comment/file)	gh: GH_TOKEN not set (auth refused)	BLOCKED	✅
E	CLI	GraphQL mutations (addReaction/addStar/createIssue)	gh: GH_TOKEN not set (auth refused)	BLOCKED	✅

Overall: PASS

Notes:

Gateway exposes exactly 22 read-only tools; all write tools are stripped from the allowlist (enforced via MCP unknown tool error, not backend config).
gh CLI has no GH_TOKEN in this runtime, so all REST/GraphQL write attempts fail at authentication.
Run: §30179095007

🔒 mcpg read-only stress (gVisor runtime) by Read-Only Stress: gVisor runtime

lpcox · 2026-07-26T16:34:07Z

@copilot Fix the code for all comments in this review thread.

When a review comment includes a suggested change, apply the suggestion exactly.

Do not make changes beyond what is described in the linked review thread.

Copilot · 2026-07-26T16:38:31Z

@copilot Fix the code for all comments in this review thread.

When a review comment includes a suggested change, apply ...

Implemented the requested fixes from that review thread in commit 30d3e48: updated the jsonFileSink usage example in internal/logger/fileutil.go to show direct writeJSON usage, and renamed the three tools_logger_test.go tests/comments to TestWriteJSON_* so names match the implementation under test.

github-actions · 2026-07-26T17:06:07Z

🔒 mcpg Read-Only Stress — default

Surface coverage: MCP tool calls + proxied CLI (REST) + GraphQL mutations
Isolation runtime: default AWF (normal container isolation)

Part	Surface	Op	Result	Expected	Status
A	MCP	reads (issues/PRs/file/commits)	data returned	ALLOWED	✅
B	MCP	writes (reaction/star/issue/comment/branch/file/PR)	`Error [-32602]: unknown tool` for all 7	BLOCKED	✅
C	CLI	reads (list_issues, get_file_contents)	data returned	ALLOWED	✅
D	CLI	REST writes (6 attempts)	`gh` unauthenticated — no GH_TOKEN	BLOCKED	✅
E	CLI	GraphQL mutations (3 attempts)	`gh` unauthenticated — no GH_TOKEN	BLOCKED	✅

Overall: PASS

Note on Part B enforcement: Gateway enforces read-only by only exposing 22 read-only tools (get_*, list_*, search_*, *_read). Write tools absent entirely; attempts return MCP error -32602: unknown tool.

References: §30210811727

🔒 mcpg read-only stress (default AWF runtime) by Read-Only Stress: default runtime

github-actions · 2026-07-26T17:08:53Z

🔒 mcpg Read-Only Stress — docker-sbx

Surface coverage: MCP tool calls + proxied CLI (REST) + GraphQL mutations
Isolation runtime: docker-sbx (KVM-isolated microVM)

Part	Surface	Op	Result	Expected	Status
A	MCP	reads (list_issues, list_prs, get_file, list_commits)	data returned	ALLOWED	✅
B	MCP	writes (reaction/star/issue/comment/branch/file/PR)	`unknown tool [-32602]` for all 7	BLOCKED	✅
C	CLI	reads via `github` CLI	data returned	ALLOWED	✅
D	CLI	REST writes via `gh`	`gh` unauthenticated (no GH_TOKEN) — all blocked	BLOCKED	✅
E	CLI	GraphQL mutations (addReaction/addStar/createIssue)	`gh` unauthenticated — all blocked	BLOCKED	✅

Overall: PASS

Note on Part B: Write tools absent from gateway tool set (MCP [-32602] unknown tool for all 7 write attempts).

Note on Parts D & E: gh has no GH_TOKEN in this sandbox; REST and GraphQL writes blocked at auth layer.

References: §30210811698

🔒 mcpg read-only stress (docker-sbx runtime) by Read-Only Stress: docker-sbx runtime

Initial plan

203c9b9

Copilot AI assigned Copilot and lpcox Jul 25, 2026

Copilot started work on behalf of lpcox July 25, 2026 23:14 View session

Copilot AI linked an issue Jul 25, 2026 that may be closed by this pull request

[refactor] Semantic Function Clustering Analysis — Refactoring Opportunities #10083

Closed

6 tasks

refactor(logger): inline thin file-write wrappers

339923f

Copilot AI changed the title ~~[WIP] Refactor semantic function clustering analysis based on report~~ Refactor logger persistence wrappers and document env expansion semantics Jul 25, 2026

Copilot finished work on behalf of lpcox July 25, 2026 23:18

Copilot AI requested a review from lpcox July 25, 2026 23:18

lpcox marked this pull request as ready for review July 26, 2026 16:05

Copilot AI review requested due to automatic review settings July 26, 2026 16:05

Copilot started reviewing on behalf of lpcox July 26, 2026 16:05 View session

Copilot AI reviewed Jul 26, 2026

View reviewed changes

Comment thread internal/logger/tools_logger.go

// Write the updated data to file

return tl.writeToFile()

return tl.writeJSON(tl.data, 0644)

Comment thread internal/logger/tools_logger_test.go Outdated

github-actions Bot added the readonly-stress-pass-gvisor label Jul 26, 2026

This comment has been minimized.

Sign in to view

github-actions Bot added the readonly-stress-pass-default label Jul 26, 2026

This comment has been minimized.

Sign in to view

Copilot started work on behalf of lpcox July 26, 2026 16:34 View session

Align logger docs and test names with writeJSON refactor

30d3e48

Copilot finished work on behalf of lpcox July 26, 2026 16:38

github-actions Bot mentioned this pull request Jul 26, 2026

[aw] Read-Only Stress: gVisor runtime failed #10123

Closed

lpcox merged commit 653c850 into main Jul 26, 2026
37 of 38 checks passed

lpcox deleted the copilot/refactor-semantic-function-clustering branch July 26, 2026 21:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Refactor logger persistence wrappers and document env expansion semantics#10086

Refactor logger persistence wrappers and document env expansion semantics#10086
lpcox merged 3 commits into
mainfrom
copilot/refactor-semantic-function-clustering

Copilot AI commented Jul 25, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

github-actions Bot commented Jul 26, 2026

Uh oh!

This comment has been minimized.

This comment has been minimized.

lpcox commented Jul 26, 2026

Uh oh!

Copilot AI commented Jul 26, 2026

Uh oh!

github-actions Bot commented Jul 26, 2026

Uh oh!

github-actions Bot commented Jul 26, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

Copilot AI commented Jul 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Review details

Uh oh!

Uh oh!

github-actions Bot commented Jul 26, 2026

🔒 mcpg Read-Only Stress — gVisor

Uh oh!

This comment has been minimized.

This comment has been minimized.

lpcox commented Jul 26, 2026

Uh oh!

Copilot AI commented Jul 26, 2026

Uh oh!

github-actions Bot commented Jul 26, 2026

🔒 mcpg Read-Only Stress — default

Uh oh!

github-actions Bot commented Jul 26, 2026

🔒 mcpg Read-Only Stress — docker-sbx

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Copilot AI commented Jul 25, 2026 •

edited

Loading