Repository navigation
Decouple shared schema validation from MCP scripts - #69490
Conversation
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
✅ PR Code Quality Reviewer completed the code quality review.
|
|
✅ 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.
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality SentinelOutcome: 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. Design Decision Gate 🏗️Outcome: 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.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design (triage: refactor_cleanup, high-impact files: schema-validation.cjs, session_parsers.go, setup.sh, mcp_scripts_mcp_server.test.cjs). This is a clean, mechanical rename with no behavior change — approving with one non-blocking suggestion.
📋 Key Themes & Highlights
Key Themes
- Mostly complete decoupling: all
require("./mcp_scripts_validation.cjs")call sites, the embed directive insession_parsers.go, the runtime copy lists insetup.sh, and the spec doc links were all updated consistently — no stale references to the old filename remain anywhere in the tree. - One stale symbol name:
GetMCPScriptsValidationScript(inpkg/workflow/js.go/pkg/cli/mcp_inspect_mcp_scripts_files.go) still encodes the old MCP-scripts vocabulary even though it now returns the content forschema-validation.cjs. Flagged inline — small, non-blocking naming follow-up that would complete the stated goal of the PR. - Updated doc comment is good: the module header in
schema-validation.cjsnow accurately describes the shared consumers (MCP servers, safe outputs, ledgers, JSON-schema consumers), matching the PR's stated intent.
Positive Highlights
- ✅ Comprehensive sweep across 22 files including Go embed directives, shell scripts, docs, and tests — no orphaned references found via repo-wide grep.
- ✅ Validation behavior and exports are genuinely unchanged; this is a pure rename plus doc update, matching the PR description exactly.
- ✅
setup.shreorders the entry to a more semantically appropriate neighborhood (next tojson_schema_validation.cjs) rather than just renaming in place.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 45 AIC · ⌖ 14.8 AIC · ⊞ 10.1K
Comment /matt to run again
| {"mcp_scripts_config_loader.cjs", workflow.GetMCPScriptsConfigLoaderScript()}, | ||
| {"mcp_server_core.cjs", workflow.GetMCPServerCoreScript()}, | ||
| {"mcp_scripts_validation.cjs", workflow.GetMCPScriptsValidationScript()}, | ||
| {"schema-validation.cjs", workflow.GetMCPScriptsValidationScript()}, |
There was a problem hiding this comment.
[/codebase-design] The PR's whole premise is decoupling this validation module from the "MCP scripts" namespace, but the Go accessor is still named GetMCPScriptsValidationScript — the one place readers connect the file content to its symbol.
💡 Why this matters
Anyone grepping for the new schema-validation.cjs file by its domain concept ("schema validation") won't find this accessor, since it still encodes the old "mcp scripts" vocabulary. Renaming to e.g. GetSchemaValidationScript (in pkg/workflow/js.go:53 and this call site) would complete the decoupling the PR description promises and keep the symbol name aligned with the file name, avoiding a confusing mismatch for future maintainers who search by either name.
@copilot please address this.
There was a problem hiding this comment.
Verdict
I didn't find a changed-line issue strong enough to block this rename.
Review scope
Checked the rename across runtime copy lists, embedded sources, tests, and spec references. I also searched for stale mcp_scripts_validation.cjs references in the repository and didn't find any remaining callers. Static review only: I did not execute tests or builds in this review workflow.
🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt54 · 52.5 AIC · ⌖ 5.64 AIC · ⊞ 12.7K
Comment /review to run again
There was a problem hiding this comment.
🟢 Approval recommended
The behavior-preserving rename has no identified blocking defects; remaining feedback concerns naming consistency.
No open issues 🎉
What changed in this PR
Renames the shared validation module to reflect its use beyond MCP scripts, preserving validation behavior and exports.
Changes:
- Rename the validation module and its tests.
- Update consumers, runtime packaging, embedding, and fixtures.
- Refresh documentation references.
| File | Description |
|---|---|
pkg/workflow/shared_go_make_test.go |
Update validation import. |
pkg/workflow/safe_output_blank_fields_test.go |
Update fixture import. |
pkg/workflow/agy_harness_test.go |
Copy renamed dependency. |
pkg/cli/mcp_inspect_mcp_scripts_files.go |
Update emitted filename. |
docs/src/content/docs/specs/mcp-scripts-specification.md |
Refresh module and test references. |
actions/setup/sh/start_mcp_scripts_server.sh |
Update required dependency. |
actions/setup/setup.sh |
Update runtime copy lists. |
actions/setup/session_parsers.go |
Embed renamed module. |
actions/setup/js/schema-validation.test.cjs |
Rename suite and imports. |
actions/setup/js/schema-validation.cjs |
Rename module; clarify shared purpose. |
actions/setup/js/safe_outputs_mcp_error_messages.test.cjs |
Update validation import. |
actions/setup/js/safe_outputs_handlers.cjs |
Update validation dependency. |
actions/setup/js/safe_output_type_validator.cjs |
Update validation dependency. |
actions/setup/js/push_ledger_changes.cjs |
Update ledger validation import. |
actions/setup/js/mcp_server_core.cjs |
Update shared validation import. |
actions/setup/js/mcp_scripts_mcp_server.test.cjs |
Copy renamed fixture dependency. |
actions/setup/js/mcp_scripts_mcp_server_http.cjs |
Update HTTP validation import. |
actions/setup/js/ledger_transactions.cjs |
Update transaction validation import. |
actions/setup/js/ledger_store.cjs |
Update store validation import. |
actions/setup/js/ledger_builtin.cjs |
Update operation validation import. |
actions/setup/js/json_schema_validation.cjs |
Update shared schema dependency. |
actions/setup/js/create_ledger_projection.cjs |
Update projection validation import. |
Review effort: Max
|
🎉 This pull request is included in a new release. Release: |
Summary
Validation helpers are shared by MCP servers, safe outputs, ledgers, and JSON-schema consumers, so they should not live in an MCP-scripts-specific module.
Rename
mcp_scripts_validation.cjstoschema-validation.cjsand update every consumer, runtime copy list, embedded source reference, test fixture, and documentation link. Validation behavior and exports remain unchanged.Validation
make fmt, JavaScript lint, and TypeScript checking.writeMCPScriptsFilesis 97 lines long, exceeding the 60-line limit. Its body is unchanged apart from the validation filename.