Skip to content

Add gojq iteration-after-error regression tests and include cached type in filter cache error - #11788

Merged
lpcox merged 3 commits into
mainfrom
copilot/go-fan-gojq-review
Aug 24, 2026
Merged

lpcox merged 3 commits into
mainfrom
copilot/go-fan-gojq-review

Conversation

Copilot AI commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

The gojq module review flagged two optional follow-ups: no dependency work is needed (already pinned to v0.12.19), but runJqCode structurally exercises the "continue iterating after an error value" pattern that upstream fixed in v0.12.19, and the defensive filter-cache type-assertion error is hard to debug.

Changes

  • internal/middleware/jqschema.go — the "should never happen" cache type-assertion failure in compileToolResponseFilterInternal now reports the actual cached type:

    return nil, fmt.Errorf("internal error: unexpected cached value type %T for filter (len=%d)", cached, len(filter))

    jqschema_gap_coverage_test.go assertion updated to match.

  • internal/middleware/jqschema_iter_after_error_test.go (new) — pins the interaction between runJqCode's CheckMultipleResults follow-up iter.Next() and gojq's error values:

    • draining a raw gojq iterator after an invalid path error (path(1), path(.a)) does not panic — direct canary for the upstream v0.12.19 fix;
    • error-as-first-result surfaces as a tool response filter error;
    • valid-value-then-invalid-path-error (path(.a), path(1)) returns the "returned multiple results" contract error without panicking.

No behavior change beyond the richer error message; the rest is test coverage.

Copilot AI linked an issue Aug 24, 2026 that may be closed by this pull request
Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
Copilot AI changed the title [WIP] Review go module gojq for security and performance Add gojq iteration-after-error regression tests and include cached type in filter cache error Aug 24, 2026
Copilot AI requested a review from lpcox August 24, 2026 14:49
@lpcox
lpcox marked this pull request as ready for review August 24, 2026 14:53
Copilot AI balanced review requested due to automatic review settings August 24, 2026 14:53

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 gojq regression coverage and improves cache type-assertion diagnostics.

Changes:

  • Reports the unexpected cached value’s concrete type.
  • Tests iterator behavior after invalid-path errors.
Show a summary per file
File Description
internal/middleware/jqschema.go Improves cache error context.
internal/middleware/jqschema_iter_after_error_test.go Adds gojq regression tests.
internal/middleware/jqschema_gap_coverage_test.go Updates the expected error message.

Review details

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

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

Comment thread internal/middleware/jqschema_iter_after_error_test.go Outdated
@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

🔒 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 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: unknown tool -32602 BLOCKED ⚠️
C CLI list_issues via github CLI data returned ALLOWED
C CLI get_file_contents via github CLI data returned ALLOWED
D CLI REST writes (reaction/star/issue/comment/file) unauthenticated (no GH_TOKEN) BLOCKED ⚠️
E CLI GraphQL mutations (addReaction/addStar/createIssue) unauthenticated (no GH_TOKEN) BLOCKED ⚠️

Overall: INCONCLUSIVE

⚠️ Part B: All 7 write tool targets (add_issue_comment, star_repository, issue_write, create_branch, create_or_update_file, create_pull_request) were absent from the 23-tool catalog — the backend runs with GITHUB_READ_ONLY=1, stripping write tools before they reach the gateway's DIFC/guard enforcement layer. Error: unknown tool -32602 for all. Gateway-level enforcement could not be independently confirmed; this is a structural limitation of the tools.github: wrapper, not a gateway gap. No write succeeded.

⚠️ Parts D/E: gh is unauthenticated in this environment (GH_TOKEN not set). All 9 write attempts (6 REST + 3 GraphQL) returned "To use GitHub CLI in a GitHub Actions workflow, set the GH_TOKEN environment variable." — the token-scope boundary could not be tested. No write succeeded.

No write leaked through in any part. The read-only guarantee held for the gVisor runtime.

References: §32742125989

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

@github-actions

Copy link
Copy Markdown
Contributor

🔒 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 (list_issues/PRs/commits, get_file_contents) data returned ALLOWED
B MCP writes (reaction/star/issue/comment/branch/file/PR) all 7 tools absent from catalog BLOCKED ⚠️
C CLI reads (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 — The MCP tool catalog exposed by the gateway contains 23 read-only tools only (get_commit, get_file_contents, issue_read, list_*, search_*, pull_request_read). All 7 targeted write tools are absent. Per the test methodology, this proves GITHUB_READ_ONLY=1 backend defense-in-depth but does not independently confirm gateway-level DIFC/guard enforcement (a write call never reaches a write-capable backend). Result: INCONCLUSIVE, not FAIL.

⚠️ Parts D & Egh CLI is not authenticated in this environment (no GH_TOKEN). REST and GraphQL write probes cannot be executed. Token-scope boundary cannot be validated. Result: INCONCLUSIVE.

No write leaked — all reads succeeded, no artifact was created.

Run: §32742125831

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

@github-actions

Copy link
Copy Markdown
Contributor

🔒 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) "unknown tool" 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 absent from catalog (GITHUB_READ_ONLY=1 backend; tools never registered). This confirms gh-aw framework defense-in-depth but cannot independently confirm mcpg's own DIFC/guard enforcement layer since no write-capable tool reached the gateway.

⚠️ Parts D & Egh CLI token is invalid (Bad credentials). All REST and GraphQL write attempts returned 401. Not authenticated, so these refusals reflect token invalidity, not confirmed gateway or token-scope enforcement. No writes leaked.

No writes succeeded. Result file: RESULT=INCONCLUSIVE RUNTIME=docker-sbx RUNID=32742125869

References: §32742125869

🔒 mcpg read-only stress (docker-sbx runtime) by Read-Only Stress: docker-sbx 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.

[go-fan] Go Module Review: gojq

3 participants