Skip to content

[test] Add tests for guard.ConfigureGlobalCompilationCache - #11684

Merged
lpcox merged 2 commits into
mainfrom
add-tests-guard-ConfigureGlobalCompilationCache-01548e8eef6a327a
Aug 22, 2026
Merged

lpcox merged 2 commits into
mainfrom
add-tests-guard-ConfigureGlobalCompilationCache-01548e8eef6a327a

Conversation

@github-actions

Copy link
Copy Markdown
Contributor

Test Coverage Improvement: ConfigureGlobalCompilationCache

Function Analyzed

  • Package: internal/guard
  • Function: ConfigureGlobalCompilationCache
  • Previous Coverage: 95.2%
  • New Coverage: 100.0%
  • Complexity: High (manages shared process-level state under a mutex, multiple error-handling branches, resource cleanup with error joining)

Why This Function?

ConfigureGlobalCompilationCache replaces the process-level wazero WASM compilation cache and is security/reliability critical: it must correctly release the previous cache and roll back cleanly on failure. While the function was already well tested (95.2%), one branch was completely uncovered: the case where both the previous cache's Close() and the newly-created replacement cache's Close() fail. This path uses errors.Join to combine both errors and must leave the global cache state pointing at the (still-open) previous cache. This is exactly the kind of tricky double-failure error-handling logic that's easy to get wrong and valuable to lock in with a test.

Implementation Note

newCompilationCache is called internally and there was no existing way to inject a cache whose Close() fails for the replacement cache (only the previous cache could be swapped directly via the existing globalCompilationCache package variable in tests). I added a minimal, behavior-preserving test seam: a package-level function variable newCompilationCacheFn that defaults to newCompilationCache and is only reassigned in tests. Production code path is unchanged.

Tests Added

  • ✅ New sub-test joins errors when both previous and replacement cache close fail in TestWasmGuardCompilationCache (table-driven t.Run style consistent with existing tests in the file)
  • ✅ Verifies errors.Join output contains both underlying error messages
  • ✅ Verifies both the previous and replacement mock caches had Close() invoked
  • ✅ Verifies the global cache is left pointing at the original (previous) cache, not swapped, since the operation failed

Coverage Report

Before: internal/guard/wasm_lifecycle.go:123: ConfigureGlobalCompilationCache  95.2%
After:  internal/guard/wasm_lifecycle.go:123: ConfigureGlobalCompilationCache  100.0%

Package internal/guard overall: 95.2% -> 96.5%

Test Execution

All tests pass:

=== RUN   TestWasmGuardCompilationCache
--- PASS: TestWasmGuardCompilationCache (0.01s)
    --- PASS: TestWasmGuardCompilationCache/joins_errors_when_both_previous_and_replacement_cache_close_fail (0.00s)
    ... (all other existing subtests pass unchanged)
PASS
ok  	github.com/github/gh-aw-mcpg/internal/guard	0.022s

go vet ./internal/guard/... and gofmt -l internal/guard/ both clean.


Generated by Test Coverage Improver
Next run will target the next most complex under-tested function

Generated by Test Coverage Improver · auto · 219.5 AIC · ⊞ 10.6K ·

Covers the previously-untested branch where both the previous
compilation cache Close() and the replacement cache Close() fail,
verifying errors.Join wraps both errors and the global cache state
is left unchanged. Introduces a minimal newCompilationCacheFn
indirection to allow test injection of a failing replacement cache.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@lpcox
lpcox marked this pull request as ready for review August 22, 2026 16:49
Copilot AI balanced review requested due to automatic review settings August 22, 2026 16:49

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Adds targeted coverage for dual cache-close failures in WASM cache replacement.

Changes:

  • Adds an injectable cache factory for testing.
  • Tests cleanup, retained global state, and combined errors.
Show a summary per file
File Description
internal/guard/wasm_lifecycle.go Adds the test seam.
internal/guard/wasm_test.go Covers dual close failures.

Review details

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

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

Comment thread internal/guard/wasm_test.go Outdated
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
@github-actions

Copy link
Copy Markdown
Contributor Author

🔒 mcpg Read-Only Stress — default AWF

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) tools absent BLOCKED ⚠️
C CLI reads (issues/PRs/file/commits) data returned ALLOWED
D CLI REST writes (reaction/star/issue/comment/file) unauthenticated BLOCKED ⚠️
E CLI GraphQL mutations (addReaction/addStar/createIssue) unauthenticated BLOCKED ⚠️

Overall: INCONCLUSIVE

Notes:

  • ⚠️ Part B (all 7 targets): Write tools absent from the MCP tool catalog — GITHUB_READ_ONLY=1 is unconditionally set by the gh-aw framework, so the GitHub MCP backend never registers write tools. This is defense-in-depth but does not independently confirm gateway-level DIFC enforcement. Gateway DIFC enforcement is covered by internal/guard and internal/difc unit tests.
  • ⚠️ Part D/E: gh CLI is not authenticated in this sandbox (no GH_TOKEN). All REST/GraphQL write attempts returned auth errors — this proves token-scope enforcement is untestable in this run, not that writes are blocked by the gateway. No writes leaked.
  • No writes succeeded in any part; no artifacts created.

Run: §32587684434

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

@github-actions

Copy link
Copy Markdown
Contributor Author

🔒 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/file/commits) data returned ALLOWED
B MCP writes (reaction/star/issue/comment/branch/file/PR) tool not found BLOCKED ⚠️
C CLI reads (issues/file) data returned ALLOWED
D CLI REST writes (reaction/star/issue/comment/file) 401 Bad credentials BLOCKED ⚠️
E CLI GraphQL mutations (addReaction/addStar/createIssue) 401 Bad credentials BLOCKED ⚠️

Overall: INCONCLUSIVE

⚠️ Part B: All 7 write tools were absent from the MCP tool catalog (catalog has 23 read-only tools). The backend runs with GITHUB_READ_ONLY=1, so write tools are never registered. This confirms the backend defense-in-depth layer works, but does not confirm mcpg's own DIFC/guard enforcement layer independently. No writes leaked (no FAIL).

⚠️ Parts D/E: gh token is invalid (Bad credentials on all calls). Cannot confirm the token-scope boundary for REST/GraphQL writes this run. All write attempts returned 401 — no writes leaked.

No writes leaked. Run is INCONCLUSIVE (not FAIL).

References:

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

@github-actions

Copy link
Copy Markdown
Contributor Author

🔒 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_pull_requests / get_file_contents / list_commits) data returned ALLOWED
B MCP writes (reaction/star/issue/comment/branch/file/PR) all 7 "unknown tool" (-32602) BLOCKED ⚠️
C CLI reads (github list_issues / get_file_contents) data returned ALLOWED
D CLI REST writes (reaction/star/issue/comment/file) gh unauthenticated BLOCKED ⚠️
E CLI GraphQL mutations (addReaction/addStar/createIssue) gh unauthenticated BLOCKED ⚠️

Overall: INCONCLUSIVE

⚠️ Part B — All 7 write tools (add_issue_comment, star_repository, issue_write, create_branch, create_or_update_file, create_pull_request) were absent from the 23-tool read-only catalog (Error -32602: "unknown tool"). Blocking proved by backend toolset exclusion (gh-aw tools.github: forces GITHUB_READ_ONLY=1), not independently by gateway/DIFC enforcement. Per test methodology this is INCONCLUSIVE — no write leaked.

⚠️ Parts D/Egh CLI is not authenticated in this environment (GH_TOKEN not set). All REST and GraphQL write attempts returned "To use GitHub CLI in a GitHub Actions workflow, set the GH_TOKEN environment variable." Token-scope boundary could not be validated this run; no write leaked.

No writes leaked in any part. Zero sentinel issues/branches/comments created.

References: §32587684415

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants