Repository navigation
feat(mcpx): make script.check canonical with legacy aliases - #5660
Conversation
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 31 minutes. View limit details
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 26d884c611
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| Long: "Resolves and checks every import without executing anything. Costs " + | ||
| "a second or two on a cold module cache, which is why it is off by " + | ||
| "default rather than on.", | ||
| Path: "script.check", PathAliases: []string{"script.typecheck"}, |
There was a problem hiding this comment.
Replace legacy config entries when persisting the canonical path
When an existing project config contains {"script":{"typecheck":"on"}}, running mcpx settings set script.typecheck off --persist project against a running daemon now leaves the configuration with conflicting values. handleSettingsSet in internal/daemon/routes_settings.go writes decl.Path, now script.check, while config.SetKey only updates that leaf without removing script.typecheck. Subsequent invocations fail settings resolution because ApplyFile reads both spellings and Set.Apply rejects their differing values at the same layer. Persistent writes need to replace or remove existing alias entries when writing the canonical setting.
Useful? React with 👍 / 👎.
| Long: "Resolves and checks every import without executing anything. Costs " + | ||
| "a second or two on a cold module cache, which is why it is off by " + | ||
| "default rather than on.", | ||
| Path: "script.check", PathAliases: []string{"script.typecheck"}, |
There was a problem hiding this comment.
Resolve legacy paths before matching remote settings rows
With a daemon running, mcpx settings get script.typecheck --daemon now reports that the daemon has no such setting, even with a fresh configuration. The remote branch in internal/cli/settings_cmd.go:110–126 fetches the settings list and compares each returned r.Path against the literal requested path, but handleSettingsList returns only the canonical script.check spelling. Unlike the local branch, this lookup never resolves the new alias, breaking existing callers of the legacy path. Canonicalize the requested path before comparison or use the daemon’s individual-setting endpoint, which already supports aliases.
Useful? React with 👍 / 👎.
Summary
script.checkis now the canonical setting. Legacyscript.typecheck,MCPX_SCRIPT_TYPECHECK,--script-typecheck, and--typecheckresolve to it; the old valueonmaps totypes. Settings are stored and read under the canonical path, including config, environment, runtime overrides, CLI, and remote exec. The default remainsoff; issue #79 owns changing it in the same release as the alias.Evidence
script.checkpath; the registry exposed onlyscript.typecheck(off | on | strict).TestPathAliasNormalizesAndStoresCanonicalPathFromEverySourceandTestScriptTypecheckOnRefusesATypeErrorcover compatibility;TestEveryExecFlagChangesSomethingverifies local and remote CLI behavior.internal/settings,internal/preflight, andinternal/e2etests; package compile checks forinternal/cli,internal/execsvc,internal/daemon,internal/api, andinternal/e2e;git diff --check.Remaining in #1911
This is the compatibility foundation, not the whole check pipeline. Follow-up order is: (1) preflight syntax/types behavior and
checkOnError/checkOnUnsupportedpolicies; (2)compileandcheckJs; (3) show the selected runtime in text output; (4) one-time legacy-spelling notice and the coordination note on #1868.Merge Danger
Door: two-way; the path and value aliases are reversible.
Blast Radius: contained to mcpx settings and existing check option propagation; no default behavior change.
Open in [code]smith · Need help on this PR? Tag
@codesmith-botwith what you need.