Centralize durable atomic file writes by Copilot · Pull Request #12653 · github/gh-aw-mcpg

Copilot · 2026-09-07T14:25:45Z

Logger persistence and delegation recovery each implemented their own temp-file/rename flow, with inconsistent durability and collision guarantees.

Shared helper
- Added internal/util.AtomicWriteFile, using a same-directory temporary file, explicit permissions, Sync, Close, and atomic Rename.
- Cleans up temporary files on failure.
Call-site migration
- Updated logger JSON persistence to use the shared helper.
- Updated delegation state persistence while preserving secure 0600 permissions and crash-safe writes.
Coverage
- Added tests for content, permissions, rename failures, cleanup, and missing parent directories.

if err := util.AtomicWriteFile(path, data, 0o600); err != nil {
    return fmt.Errorf("failed to persist delegation state: %w", err)
}

Fixes [duplicate-code] Duplicate Code Pattern: Atomic temp-file+rename write reimplements internal/logger/fileutil.go #12631

Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>

Copilot

🟡 Changes recommended

The helper does not sync parent-directory metadata and silently ignores cleanup failures.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Centralizes atomic persistence for logger data and delegation recovery.

Changes:

Adds a shared synced temporary-file/rename helper.
Migrates logger and delegation persistence and adds coverage.

File summaries

File	Description
`internal/util/atomic_write.go`	Implements shared atomic writes.
`internal/util/atomic_write_test.go`	Tests content, permissions, and failures.
`internal/logger/fileutil.go`	Delegates logger writes to the helper.
`internal/logger/atomic_write_test.go`	Updates logger atomic-write tests.
`internal/delegation/recovery.go`	Migrates secure state persistence.

Review details

Files reviewed: 5/5 changed files
Comments generated: 2
Review effort level: Balanced

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

+	if err := os.Rename(tempPath, filePath); err != nil {
+		return fmt.Errorf("failed to rename temp file: %w", err)
+	}


+import (
+	"fmt"
+	"os"
+	"path/filepath"
+)
+
+// AtomicWriteFile writes data to filePath by syncing a uniquely named
+// temporary file in the destination directory before renaming it into place.
+func AtomicWriteFile(filePath string, data []byte, perm os.FileMode) error {
+	temp, err := os.CreateTemp(filepath.Dir(filePath), "."+filepath.Base(filePath)+"-*")
+	if err != nil {
+		return fmt.Errorf("failed to write temp file: %w", err)
+	}
+	tempPath := temp.Name()
+	defer os.Remove(tempPath)


lpcox · 2026-09-07T14:51:43Z

@copilot address review feedback

Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>

Copilot · 2026-09-07T14:58:52Z

@copilot address review feedback

Addressed in f19c332: parent-directory sync now runs after rename, cleanup failures are preserved with the primary error, and focused cleanup coverage was added.

github-actions · 2026-09-07T15:11:25Z

🔒 mcpg Read-Only Stress — default

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

Part	Surface	Op	Result	Expected	Status
A	MCP	reads (list_issues, list_pull_requests, get_file_contents, list_commits)	data returned	ALLOWED	✅
B	MCP	writes (add_issue_comment, star_repository, issue_write, create_branch, create_or_update_file, create_pull_request)	all 6 absent from the 23-tool catalog exposed by `github` CLI proxy	BLOCKED	⚠️
C	CLI	reads (same as A, via `github` CLI proxy)	data returned	ALLOWED	✅
D	CLI	REST writes (reaction/star/issue/comment)	not attempted — `gh auth status` shows unauthenticated	BLOCKED	⚠️
E	CLI	GraphQL mutations (addReaction/addStar/createIssue)	not attempted — `gh` unauthenticated	BLOCKED	⚠️

Overall: INCONCLUSIVE

Notes:

No write leaked through any surface — zero reaction/star/issue/comment/branch/file/PR created.
Part B tools were entirely absent from the exposed MCP tool catalog (read-only toolset, 23 tools total, no mutation tools). Per methodology this only proves backend/toolset configuration, not the gateway's own DIFC/guard blocking layer, hence ⚠️ rather than ✅.
Parts D/E could not be exercised because gh is not authenticated in this job (You are not logged into any GitHub hosts), so the token-scope boundary was not independently tested this run.
No security regression evidence; reporting INCONCLUSIVE rather than PASS strictly per the stated validation criteria.

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

github-actions · 2026-09-07T15:12:26Z

🔒 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 (issues/PRs/README/commits)	data returned	ALLOWED	✅
B	MCP	writes (reaction/star/issue/comment/branch/file/PR)	all 6 write tools absent from the 23-tool catalog (read-only backend)	BLOCKED	⚠️ INCONCLUSIVE — tools not exposed at all, so gateway's own denial path wasn't exercised
C	CLI	reads (issues/PRs/README)	data returned	ALLOWED	✅
D	CLI	REST writes	not attempted — `gh auth status` reports token invalid/unauthenticated	BLOCKED	⚠️ INCONCLUSIVE
E	CLI	GraphQL mutations	not attempted — `gh` unauthenticated	BLOCKED	⚠️ INCONCLUSIVE

Overall: INCONCLUSIVE

No write leaked in any part. Gaps: (1) the gateway-backed github CLI only exposes 23 read-only tools — none of the targeted write tool names exist in the catalog, so Part B could only confirm backend read-only config, not the gateway's independent DIFC/guard denial path. (2) gh CLI is not authenticated in this environment (GH_TOKEN invalid), so Parts D/E (REST/GraphQL write rejection via token scope) could not be exercised. Recommend a follow-up probe that launches the backend outside gh-aw's tools.github: read-only wrapper to independently confirm mcpg's own write-blocking layer, and ensuring gh has a valid read-scoped token for future runs.

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

github-actions · 2026-09-07T15:12:43Z

🔒 mcpg Read-Only Stress — gvisor

Surface coverage: MCP tool calls + proxied CLI (REST) + GraphQL mutations
Isolation runtime: gvisor

Part	Surface	Op	Result	Expected	Status
A	MCP	reads (list_issues, list_pull_requests, get_file_contents, list_commits)	data returned	ALLOWED	✅
B	MCP	writes (issue_write/star_repository/create_branch/create_or_update_file/create_pull_request/add_issue_comment)	`unknown tool` (-32602) — absent from 23-tool catalog, not a gateway-specific refusal	BLOCKED	⚠️
C	CLI	reads via `github` proxy on PATH	data returned	ALLOWED	✅
D	CLI	REST writes (reaction/star/issue/comment)	not attempted — `gh` unauthenticated ("not logged into any GitHub hosts")	BLOCKED	⚠️
E	CLI	GraphQL mutations (addReaction/addStar/createIssue)	not attempted — `gh` unauthenticated	BLOCKED	⚠️

Overall: INCONCLUSIVE

No write leaked on any surface. Part B is inconclusive because gh-aw's tools.github: wrapper always sets GITHUB_READ_ONLY=1, so write tools (issue_write, star_repository, create_branch, create_or_update_file, create_pull_request) are never registered in the backend's tool catalog — confirmed via explicit calls returning unknown tool "..." rather than a gateway-level permission/DIFC refusal. This proves backend/toolset configuration blocks writes, but does not independently exercise mcpg's own DIFC/guard enforcement layer for MCP tool calls. Parts D and E are inconclusive because gh is not authenticated in this environment, so the REST/GraphQL write path could not be probed this run. Per the test's validation criteria, these gaps make the result INCONCLUSIVE rather than PASS or FAIL, since no write actually leaked through mcpg on any surface that was reachable.

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

Initial plan

0ff2ca5

Copilot AI assigned Copilot and lpcox Sep 7, 2026

Copilot started work on behalf of lpcox September 7, 2026 14:26 View session

Copilot AI linked an issue Sep 7, 2026 that may be closed by this pull request

[duplicate-code] Duplicate Code Pattern: Atomic temp-file+rename write reimplements internal/logger/fileutil.go #12631

Closed

7 tasks

Deduplicate durable atomic file writes

2ee0a48

Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>

Copilot AI changed the title ~~[WIP] Fix duplicate atomic write implementation in recovery.go~~ Centralize durable atomic file writes Sep 7, 2026

Copilot AI requested a review from lpcox September 7, 2026 14:33

Copilot finished work on behalf of lpcox September 7, 2026 14:33

lpcox marked this pull request as ready for review September 7, 2026 14:35

Copilot AI balanced review requested due to automatic review settings September 7, 2026 14:35

Copilot started reviewing on behalf of lpcox September 7, 2026 14:36 View session

Copilot AI reviewed Sep 7, 2026

View reviewed changes

This comment has been minimized.

Sign in to view

Copilot started work on behalf of lpcox September 7, 2026 14:52 View session

Harden atomic write durability cleanup

f19c332

Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>

Copilot finished work on behalf of lpcox September 7, 2026 14:59

lpcox merged commit a064acc into main Sep 7, 2026
38 checks passed

lpcox deleted the copilot/duplicate-code-fix-atomic-write branch September 7, 2026 15:15

This was referenced Sep 7, 2026

Smoke Test: Copilot - 34137645052 #12677

Closed

Smoke Test: Copilot - 34137887030 #12680

Closed

[release] Release highlights for v0.4.18 #12691

Open

Navigation Menu

Uh oh!

Centralize durable atomic file writes - #12653

Centralize durable atomic file writes#12653
lpcox merged 3 commits into
mainfrom
copilot/duplicate-code-fix-atomic-write

Copilot AI commented Sep 7, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

lpcox commented Sep 7, 2026

Uh oh!

Copilot AI commented Sep 7, 2026

Uh oh!

github-actions Bot commented Sep 7, 2026

Uh oh!

github-actions Bot commented Sep 7, 2026

Uh oh!

github-actions Bot commented Sep 7, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

Copilot AI commented Sep 7, 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

🟡 Changes recommended

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

lpcox commented Sep 7, 2026

Uh oh!

Copilot AI commented Sep 7, 2026

Uh oh!

github-actions Bot commented Sep 7, 2026

🔒 mcpg Read-Only Stress — default

Uh oh!

github-actions Bot commented Sep 7, 2026

🔒 mcpg Read-Only Stress — docker-sbx

Uh oh!

github-actions Bot commented Sep 7, 2026

🔒 mcpg Read-Only Stress — gvisor

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Copilot AI commented Sep 7, 2026 •

edited

Loading