Repository navigation
feat(mcpx): toolViews slice 1 — rename/pin/hide on alias views (#1933) - #2976
Conversation
|
Caution Review failedThe pull request is closed.
 |
| Layer / File(s) | Summary |
|---|---|
Tool-view configuration pkgs/mcpx/internal/config/config.go, pkgs/mcpx/internal/config/parse_pin_test.go, pkgs/mcpx/internal/config/profile_test.go |
Configuration adds tool-view declarations, validates their shape, and applies alias visibility rules. JSON parsing preserves exact numbers and rejects trailing data. |
Pinned schema projection pkgs/mcpx/internal/config/argumentrules.go, pkgs/mcpx/internal/config/argumentrules_test.go |
Pinned projections remove requirements satisfied by pins and project constraints involving applicators, references, dependencies, property counts, and conditions. Tests cover dialects, resources, and visible versus hidden pins. |
View listing and validation pkgs/mcpx/internal/daemon/registry.go, pkgs/mcpx/internal/daemon/reload.go, pkgs/mcpx/internal/daemon/toolviews_test.go |
The registry lists aliases with projected schemas and validates targets and pins against cached schemas. Reload validates views before replacing registry state. |
Pinned call dispatch pkgs/mcpx/internal/daemon/registry.go, pkgs/mcpx/internal/daemon/input_required.go, pkgs/mcpx/internal/daemon/routes_proto.go, pkgs/mcpx/internal/daemon/toolviews_test.go |
Call paths resolve view names to target tools, merge pinned arguments, and pass the effective tool and arguments through guards to the pool. |
Estimated code review effort: 4 (Complex) | ~60 minutes
Change: Feature
Sequence Diagram(s)
sequenceDiagram
participant Caller
participant Registry
participant Pool
Caller->>Registry: Call with view name and arguments
Registry->>Registry: Resolve target and merge pinned arguments
Registry->>Pool: Dispatch effective tool and arguments
Possibly related PRs
- developing-today/code#2829: Both changes update
resolveAndGuardand its call sites; this PR resolves view targets and pinned arguments, while PR 2829 changes write-policy checks and ask/deny handling around that resolution.
Merge Risk: 
92a21
-
Resolve the schema projection and validation defects before merging: valid views can fail to load, advertised inputs can disagree with enforcement, and larger pin sets can make listings excessively expensive. Reload consistency and misleading call diagnostics also remain outstanding.
Pre-merge checks: 
-
Check name Status Explanation Resolution Docstring Coverage Docstring coverage is 73.91% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documenta… Write docstrings for the functions missing them to satisfy the coverage threshold. -
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly identifies the main change: adding alias tool views with rename, pin, and hide behavior. It is specific and related to the changeset. Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 73.91% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 46 functions across 10 files.
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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: 8df6ccfb07
ℹ️ 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".
…load, keep diagnostics caller-facing
…iews-slice1 # Conflicts: # pkgs/mcpx/internal/daemon/registry.go
|
@codex review — main merged in (46ac651, origin/main c1b9052); the single conflict (pkgs/mcpx/internal/daemon/registry.go in NewRegistry) was resolved as a union: this branch's validateViews() error return kept, then main's warnDeadArgumentRules() call before return. Laptop targeted tests: go test ./internal/daemon/ PASS (-count=1, 47s), ./internal/config/ PASS. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 46ac651db3
ℹ️ 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".
…ntity; preserve number precision when hiding schema props Codex P2 x2 on merge head 46ac651 (queue task 3230): - Tools/CodegenNamespaces advertised EffectiveSchema under the view alias while applyWritePolicy enforces rules under the target real name; advertise via ruleToolName (alias -> From) so a rule is neither enforced-unadvertised nor advertised-unenforced. - hideSchemaProps decoded into map[string]any via float64, rewriting large integers (enum 9007199254740993 -> ...992); decode with UseNumber like argumentrules.go already does. Regression tests for both in toolviews_test.go.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2a9359d3fc
ℹ️ 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".
|
🤖 Agent note · parked
|
…r precision (#1933) Codex P2s on PR #2976, round at 2a9359d: 1. Rule projection (registry.go:881 finding): EffectiveSchemaPinned in config drops pinned names from rules required/dependentRequired lists (any depth) for the advertised projection only — pins merge before enforcement, so the pin satisfies the requirement; verbatim projection against a hidden property + additionalProperties:false advertised an unsatisfiable schema. Both catalog paths (Tools and Catalog) now go through effectiveViewSchema. Enforcement unchanged. 2. Pin precision (config.go finding): config.parse decodes with json.Number (UseNumber), so an integer pin past the float64-exact range (9007199254740993) is forwarded exactly instead of rounded to 9007199254740992. Tests: TestEffectiveSchemaPinnedDropsPinnedRequirements, TestParsePreservesPinNumberPrecision, TestAdvertisedViewSchemaDropsPinnedRuleRequirements. go test ./internal/config/ ./internal/daemon/ passes on the laptop.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ec95b52050
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a215733225
ℹ️ 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".
…en pins out of allOf; defer view validation on expired catalog (#1933) Codex P2 round on PR #2976 (queue task 3230): - config/argumentrules: stripPinnedRequirements now works at one instance scope only, following same-instance applicators (allOf/anyOf/oneOf/not/if/then/else/dependentSchemas) and never crossing into nested scopes (properties/items/$defs), so a nested property sharing a pinned name keeps its requirement. - config/argumentrules: dependentRequired entries triggered by a pinned property promote their remaining dependencies to required -- pins merge before enforcement, so those dependencies are unconditional at runtime and the advertised schema must say so. - daemon/registry: hideSchemaProps recurses into allOf members, so a hidden pin is projected out of same-instance constraints (was: unsatisfiable schema against additionalProperties:false). - daemon/registry: validateViews skips servers whose cached catalog is expired instead of letting stale contents veto a reload; validation defers to the refreshed catalog. Tests: TestEffectiveSchemaPinnedKeepsNestedScopeRequirements, TestEffectiveSchemaPinnedPromotesPinnedTriggerDependencies, TestHideSchemaPropsProjectsAllOfRequirements, TestReloadDefersViewValidationOnExpiredCatalog. go test ./internal/config/ ./internal/daemon/ passes on the laptop.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6d8011ca6d
ℹ️ 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".
…; collect composed pin declarations (#1933) Codex re-review of 6d8011c raised three P2s, all in the pinned-view projection family fixed earlier in this PR: - argumentrules: a dependentSchemas entry whose trigger is pinned now promotes its projected schema into allOf at the same instance scope (the pin always fires the trigger at enforcement, since pins merge before rules run). - argumentrules: local $ref targets are inlined and projected at the scope the reference occupies, so requirements reached through #/$defs/... no longer survive the pinned projection unexamined; resolution is cycle-guarded. - daemon/registry schemaProps: pin validation now collects property declarations through same-instance composition (allOf/anyOf/oneOf, if/then/else) and local references, so a view pinning an allOf-declared argument is not rejected as unknown. Regression tests for all three. go test ./internal/config/ ./internal/daemon/ PASS on the laptop.
|
Codex re-review requested for head 6b0ff07: the three P2s from the 6d8011c review are fixed in that commit (composed/reference pin declarations in validateViews/schemaProps; pinned dependentSchemas promotion into allOf; local $ref inlining in the pinned projection), each with a regression test. Threads replied to and resolved. @chatgpt-codex-connector please re-review. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6b0ff07ce4
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9c5578716e
ℹ️ 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".
…lues in the advertised projection (#1933) Codex P2 on PR #2976: with tailnet pinned and hidden, projecting a rule if/then/else by stripping the pinned name from the condition advertised the wrong branch -- the stripped if succeeds when the caller omits tailnet, while enforcement merges the pin (tailnet=other), fails the const, and takes else. The projection now threads the pin VALUES through: when every property a condition examines is pinned and the condition compiles and evaluates cleanly against the pin instance (schemaval, local refs inlined), the decided branch folds in as an unconditional conjunct and the condition is dropped. Conditions reading caller-supplied properties, whole-instance or name-pattern shapes stay conditional under the previous name-stripping. Behavioral regressions pin both directions (pin other -> else/cursor advertised; pin prod -> then/limit advertised) plus the undecided case.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 59e5c62849
ℹ️ 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".
|
🤖 Agent note · parked
|
|
Queue task 5609 revalidation (repo-worker-code, 2026-11-02T15:17:59Z): PR remains OPEN at head ce1d517 against main, mergeable=CONFLICTING (mergeStateStatus DIRTY). CI run 41366817770 still shows basic lint FAIL and stress changed-tests FAIL; docs-history, plugin tests, mcpx-docs build, and CI time budget pass. No merge taken: standing criteria (green checks, mergeable) are not met, and the merge-conflict + conformance-mock repair needs a dedicated window beyond this run. Parking the queue task with this resume brief; no remote or local changes made this run.\n\n |
…iews-slice1 # Conflicts: # pkgs/mcpx/internal/config/config.go # pkgs/mcpx/internal/daemon/input_required.go # pkgs/mcpx/internal/daemon/registry.go # pkgs/mcpx/internal/daemon/routes_proto.go
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 92a21ec9e6
ℹ️ 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".
| pinnedCountConjunct(kind, n, pins[1:])) | ||
| absent := conjoinCountSchemas( | ||
| map[string]any{"not": map[string]any{"required": []any{pins[0]}}}, | ||
| pinnedCountConjunct(kind, n-1, pins[1:])) |
There was a problem hiding this comment.
When a view pins several arguments and a matching rule contains minProperties or maxProperties, these two recursive calls generate every subset of pinned names on every schema listing. Running the extracted helpers with 16 pins and minProperties:1 produced a 3,930,073-byte schema in roughly 0.87 seconds, even though that constraint is always satisfied by the pins alone. Larger pin sets rapidly make catalog generation impractical or exhaust memory. Short-circuit pin-decided bounds and use a bounded-size representation for the remaining overlap-dependent constraints.
Useful? React with 👍 / 👎.
| m, ok := cur.(map[string]any) | ||
| if !ok { | ||
| return nil, false |
There was a problem hiding this comment.
For a cached tool whose root $ref is #/$defs/shared/allOf/0, with tailnet declared in that referenced member, this resolver returns false when it reaches the allOf array because every segment is required to be a map. Consequently, schemaProps reports no tailnet, and validateViews rejects an otherwise valid view pinning that argument during startup or reload. Handle numeric JSON Pointer segments in arrays rather than rejecting these valid reference targets.
Useful? React with 👍 / 👎.
| if sm, ok := sub.(map[string]any); ok { | ||
| collectSchemaProps(sm, root, out, resolving) |
There was a problem hiding this comment.
When an allOf member declares its own $id, defines $defs.need.properties.tailnet, and references #/$defs/need, this recursion retains the outer document as root. Its fragment reference therefore fails to resolve against the member's definitions, so schemaProps omits tailnet and cached-schema validation rejects a valid view pinning it. The fresh evidence beyond the earlier resource-projection fixes is this separate declaration-collection path used by validateViews; it also needs resource-aware resolution and cycle tracking.
Useful? React with 👍 / 👎.
|
🤖 Agent note · done
|
Slice 1 of #1933 (Approach B, per triage task 2829): declared
toolViewson alias (aliasOf) views. This PR references #1933 and does not close it — slice 2 (cross-server virtual namespaces) stays there.What
mcpx.toolViewsmap on a server's extras: each entry names a target tool viafrom(rename), maypinfixed arguments, mayhidepinned properties, and may overridedescription. Structural validation at resolve: toolViews requiresaliasOf,fromis required, andhidemay only list pinned properties.visibleToolsmaterialises exactly the named tools under their view names (sorted, deterministic), with the description override applied and hidden properties removed from the exposedinputSchema(properties+required). Unrenamedtools/excludeToolsbehaviour is unchanged for servers without toolViews.resolveAndGuardtranslates a view call to the target's real name with pinned arguments merged over the caller's (a pin wins), so the write policy, the destructive floor, the pool, and history/diagnostics all work with the real name and the effective args, on every call path (Call,CallReporting,CallAsk). The target's real name is not callable through the view (VisibleToolanswers in view names).fromtarget or a pin of an argument the target schema does not declare, naming the server and view.Tests
The tailscale shape from #1933 end to end at the daemon level: an alias view exposing exactly
listTailnet(fromlist_tailnets) andlistHostnames(fromlist_devices,tailnetpinned to-and hidden) — listings, schema hiding, pin merging (caller cannot override), guard behaviour (view name passes, real name is HiddenTool), plus config resolve/visibility and validation tests.Verified on the laptop worktree:
go test ./internal/config/ ./internal/daemon/— both pass in full. CI is the remaining verification.Queue task 3230 · issue #1933 (stays open for slice 2)
Need help on this PR? Tag
@codesmith-botwith what you need.Summary by CodeRabbit