Repository navigation
fix: align MCP gateway schema and reference examples - #70865
Conversation
Accept the existing trusted-bots gateway configuration with strict array validation and compile the complete trusted-bots and OIDC reference examples as regression tests. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
✅ Ponytail Reviewer completed successfully! Ponytail ReviewerOutcome: The agent completed and reported no action. A no-op output alone does not verify that all required work was completed or that publication was possible. Agent messages are omitted from this comment. The category uses structured run status, not interpretation of agent prose.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. See the comment below for the result and any generated ADR draft. Commit pushed:
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict
No blocking correctness issues found in the changed lines. The schema addition, corrected frontmatter placement, and documentation compilation coverage are consistent with the existing implementation. Static review only; tests and builds were not run.
🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt50 · 2.45 AIC · ⌖ 0.539 AIC · ⊞ 1.6K
Comment /review to run again
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /grill-with-docs and /tdd (via pr-triage). This is a tight, well-tested fix for a real schema/docs mismatch.
📋 Key Themes & Highlights
Key Themes
- Latent alias drift: The schema now validates
trusted-bots(kebab-case), matching the extraction code, but the code also accepts atrustedBotscamelCase alias (frontmatter_extraction_security.go:626) that the schema still rejects due toadditionalProperties: false. The doc prose above the first example still referencestrustedBotstoo. Flagged inline — same bug class this PR fixes, just the other spelling.
Positive Highlights
- ✅ Both reference examples are now complete, runnable workflows (frontmatter + body), not just fragments — directly addresses the discussions/69855 report.
- ✅
mcp_gateway_reference_test.goextracts the doc fences and actually compiles them, asserting on emitted lock-file content (trustedBotsarray, OIDC URL/audience,id-token: write, token env vars). This closes the loop so docs drift will fail CI going forward — exactly the kind of regression guard/grill-with-docsasks for. - ✅
schema_trusted_bots_test.gocovers the meaningful edge cases (empty list, empty string, mixed types, scalar, null) rather than just the happy path. - ✅ Verified locally: both new test files pass (
go test ./pkg/parser/... -run TestSandboxMCPTrustedBotsSchemaand./pkg/workflow/... -run TestMCPGatewayReferenceExamplesCompile).
No blocking issues — the one inline comment is a pre-existing gap adjacent to this PR's scope, worth a quick follow-up rather than holding this up.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 31.1 AIC · ⌖ 15 AIC · ⊞ 10.2K
Comment /matt to run again
🏗️ Design Decision Gate — ADR RequiredThis PR adds more than 100 new lines in business logic directories ( 📄 Draft ADR committed: The draft records two decisions:
📋 What to do next
Once an ADR is linked in the PR body, this gate will re-run and check that the implementation matches the decision. ❓ Why ADRs Matter
ADRs create a searchable, permanent record of why the codebase looks the way it does. 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
|
There was a problem hiding this comment.
🟡 Changes recommended
Accepted trusted-bot values can execute shell commands during generated gateway setup.
1 open issue
What changed in this PR
Aligns MCP gateway schema validation and documentation with existing compiler support.
Changes:
- Adds
sandbox.mcp.trusted-botsschema support. - Makes both reference examples complete workflows and corrects OIDC configuration.
- Adds schema validation and documentation-compilation regression tests.
| File | Description |
|---|---|
pkg/workflow/mcp_gateway_reference_test.go |
Compiles documentation examples and checks generated configuration. |
pkg/parser/schemas/main_workflow_schema.json |
Allows nonempty trusted-bot lists. |
pkg/parser/schema_trusted_bots_test.go |
Tests accepted and rejected trusted-bot values. |
docs/src/content/docs/reference/mcp-gateway.md |
Provides complete trusted-bot and OIDC workflows. |
Review effort: Max
Remove the unreachable camelCase frontmatter alias, validate literal identities at the schema and renderer boundaries, cover unsafe substitutions without executing them, and complete the design decision record. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
In response to #70865 (comment): No code change is needed for this completion notice because it contains no concrete finding. I validated the actual revision separately with focused schema/gateway tests and the 346-workflow drift check; the remaining lint limitations are recorded in the PR description. |
|
In response to #70865 (comment): No code change is needed for this status-only report, and reported_no_action is not being treated as validation evidence. The review fixes were independently tested and published in c921e3d, with incomplete lint coverage called out explicitly. |
|
In response to #70865 (comment): The completion notice itself requires no additional code change; the substantive alias feedback was accepted and addressed in its inline thread. Frontmatter now consistently uses trusted-bots, while gateway JSON uses trustedBots, with regression coverage for that boundary. |
|
In response to #70865 (comment): Accepted the generated ADR follow-up: I reviewed and completed the draft rather than treating this completion notice as approval. The ADR now records the final identity validation and reference-test decisions and is linked in the PR body. |
|
In response to #70865 (comment): No additional code change is required by this completion notice, which contains no concrete finding. The separate alias and shell-safety findings were addressed, and the final validation results and lint limitations are documented in the PR description. |
Summary
sandbox.mcp.trusted-botsconfiguration in the canonical workflow schema, requiring a nonempty array of safe literal identities. Validate the same identity characters at the renderer boundary to prevent shell expansion when callers bypass the schema.trustedBotsfrontmatter extraction alias. Frontmatter usessandbox.mcp.trusted-bots; generated gateway JSON usesgateway.trustedBots.mcp-serversat the frontmatter root, with the required token permission and network allowlist.Addresses the reproduced schema/example mismatches reported in #69855. This intentionally does not add unsupported
tools.github.refusal-labelsor change concurrency behavior.ADR: ADR-70865
Validation
trusted-botsschema rejection before the schema change.make buildandmake fmtpass.make agent-report-progresspassed schema freshness and impacted unit tests, but did not pass the complete lint gate: Go lint encountered another runner's global lock, and custom lint flagged existing uncheckedfmt.Fprintfcalls in the now-touched renderer. Those calls are outside the added validation lines and write to astrings.Builder.make check-workflow-driftpassed after the review fixes: all 346 workflows compile, and all lock files remain in sync. Existing workflow warnings are unchanged.Publication uses the original session branch because the requested branch rename was blocked by an existing shared
packed-refs.lock; no shared Git lock was removed.