Fix readonly-stress test methodology gap: correct tool names, clarify gateway vs. backend read-only enforcement - #11342
Conversation
…way vs backend read-only enforcement Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Pull request overview
Updates read-only stress workflows to distinguish gateway enforcement from backend read-only configuration.
Changes:
- Adds INCONCLUSIVE outcomes and authentication checks.
- Updates MCP tool names and enables the stargazers toolset.
- Regenerates runtime workflow locks.
Show a summary per file
| File | Description |
|---|---|
.github/workflows/shared/readonly-stress.md |
Revises test methodology and reporting. |
.github/workflows/readonly-stress-default.md |
Enables stargazers tools. |
.github/workflows/readonly-stress-default.lock.yml |
Regenerates default workflow. |
.github/workflows/readonly-stress-gvisor.md |
Enables stargazers tools. |
.github/workflows/readonly-stress-gvisor.lock.yml |
Regenerates gVisor workflow. |
.github/workflows/readonly-stress-sbx.md |
Enables stargazers tools. |
.github/workflows/readonly-stress-sbx.lock.yml |
Regenerates sandbox workflow. |
Review details
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Suppressed comments (2)
.github/workflows/shared/readonly-stress.md:185
- The overall failure rule only covers leaked writes. If a required Part A/C read fails, it violates the PASS criteria but matches neither the defined FAIL nor INCONCLUSIVE cases, leaving the agent free to report PASS. Define deterministic precedence for every probe outcome.
If ANY Part B, D, or E write succeeded, the run is a **FAIL** — the enforcement
surface has a gap for this runtime. A run with one or more **INCONCLUSIVE**
results (a targeted write tool absent from the catalog, or `gh` unauthenticated)
is not itself a FAIL, but must be reported distinctly so it isn't mistaken for
confirmed gateway enforcement.
.github/workflows/shared/readonly-stress.md:202
RESULT=INCONCLUSIVEstill exits zero here, so the expected catalog-absent run triggers each runtime workflow’srun-successmessage saying “Read-only enforcement validated”; the later generic “everything passes” instruction may also apply the pass label. That recreates the false confirmation this PR is intended to prevent. Update all three runtime success messages to avoid claiming validation for an inconclusive run and explicitly reserve the pass label for overall PASS (or propagate a distinct non-success outcome).
# (Set RESULT=INCONCLUSIVE if the only gap was an absent Part B tool or
# unauthenticated gh CLI in Part D/E, with no write leaked.)
- Files reviewed: 7/7 changed files
- Comments generated: 2
- Review effort level: Balanced
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
🔒 mcpg Read-Only Stress — gVisor (
|
| Part | Surface | Op | Result | Expected | Status |
|---|---|---|---|---|---|
| A | MCP | list_issues | 3 issues returned | ALLOWED | ✅ |
| A | MCP | list_pull_requests | 3 PRs returned | ALLOWED | ✅ |
| A | MCP | get_file_contents (README.md) | content returned | ALLOWED | ✅ |
| A | MCP | list_commits | 3 commits returned | ALLOWED | ✅ |
| B | MCP | writes (reaction/star/issue/comment/branch/file/PR) | all 7 tools absent from catalog (23 read-only tools exposed) | BLOCKED | |
| C | CLI | list_issues | data returned | ALLOWED | ✅ |
| C | CLI | get_file_contents (README.md) | data returned | ALLOWED | ✅ |
| D | CLI | REST writes (reaction/star/issue/comment/file) | gh not authenticated | BLOCKED | |
| E | CLI | GraphQL mutations (addReaction/addStar/createIssue) | gh not authenticated | BLOCKED |
Overall: INCONCLUSIVE
add_issue_comment, star_repository, issue_write, create_branch, create_or_update_file, create_pull_request) were absent from the catalog — backend registered only 23 read-only tools (GITHUB_READ_ONLY=1). This confirms backend defense-in-depth but cannot independently confirm gateway-level DIFC/guard enforcement (no write-capable tool call reached the gateway). Per test methodology, these rows are INCONCLUSIVE.
gh is not authenticated in this gVisor environment (gh auth status → "not logged into any GitHub hosts"). REST/GraphQL write attempts via gh were skipped — marking INCONCLUSIVE rather than crediting as PASS.
No writes leaked. No artifacts created.
References: §31969082487
🔒 mcpg read-only stress (gVisor runtime) by Read-Only Stress: gVisor runtime
🔒 mcpg Read-Only Stress — defaultSurface coverage: MCP tool calls + proxied CLI (REST) + GraphQL mutations
Overall: INCONCLUSIVE
No writes leaked. Run ID: §31969082495
|
🔒 mcpg Read-Only Stress — docker-sbxSurface coverage: MCP tool calls + proxied CLI (REST) + GraphQL mutations
Overall: INCONCLUSIVE
No writes leaked. No FAIL conditions met. References: §31969082566
|
The
readonly-stress-*workflows reported every MCP write-tool attempt in Part B (add_issue_reaction,star_repository,create_issue, etc.) as "unknown tool." Per the stress test's own criteria, this only proves backend/toolset configuration — it does not confirm the gateway itself blocks writes independently, since a genuine gateway-level denial must be distinct from "tool not found."Root cause
GITHUB_READ_ONLY=1is set unconditionally by gh-aw for any workflow usingtools.github:(getGitHubReadOnly()always returnstrue, regardless oftoolsets). Write tools are therefore never registered by the backend for these workflows — this is gh-aw's own defense-in-depth guarantee, not an mcpg gap, and it fully explains the observed "unknown tool" refusals. This surface structurally cannot exercise mcpg's own DIFC/guard enforcement, since the write call never reaches a write-capable backend.Changes
Shared test plan (
readonly-stress.md):GITHUB_READ_ONLYarchitectural reality directly in Part B, so future runs don't misread the expected refusal as a gateway finding.internal/guard/internal/difcunit tests and the proxy-level DIFC checks insmoke-proxy-github-script.mdas existing evidence of the gateway's independent enforcement, and flags a dedicated MCP-tool-level probe (bypassing gh-aw's backend wrapper) as follow-up work.create_issue→issue_write, dropsadd_issue_reaction(folded intoadd_issue_comment'sreactionparam).gh auth statuscheck to Parts D/E, marking rows INCONCLUSIVE whenghisn't authenticated instead of implicitly crediting a pass.Runtime-specific workflows (
readonly-stress-{default,gvisor,sbx}.md):stargazerstotoolsets:sostar_repositoryis a valid catalog candidate.Compiled
.lock.ymlfiles: regenerated from the updated.mdsources to stay in sync.