feat(plugin-mcp): the shape of one entity - #1858
Open
mobeenabdullah wants to merge 14 commits into
Open
Conversation
`get_initial_context` says which entities an agent can work with; these two tools say what one of them looks like. Split rather than folded into that answer, because an install with forty collections would otherwise spend every conversation's opening on thirty-nine schemas nobody asked for. The schema comes from the registry through the services facade rather than from a description assembled here. Code-first collections are synced into the same registry the Schema Builder writes to, so the registry is the merged view of both and the type generator reads the same records. Reading the config object instead would answer correctly for a code-first install, be blind to every collection an operator built in the admin, and look right in both. Authorization runs BEFORE the read. `getCollection` takes a request context and does not use it: the registry read is deliberately not access-controlled, because the registry is how the system describes itself. The gate in the tool is therefore the only thing between a scoped key and the shape of an entity it was never granted, which makes its ordering a precondition rather than a filter. The decision is core's own `canReadContent`, the same one that built the entity list, so the two tools cannot disagree about what a caller may see. A refusal does not separate "you may not read this" from "no such entity", because a caller able to tell those apart can map an install's slugs by asking about guesses. A field's name stays optional rather than being defaulted to an empty string. Presentational types carry none, and substituting one would put a field in the answer that an agent could then try to read. `canReadContent` is published from core beside `readableContent`, composed from the same per-entity decision through one shared caller conversion, and refuses an unregistered slug rather than judging it: a slug with no registry entry has no rule to decide against.
`canReadContent` refuses an unregistered slug rather than judging it, and mutation testing found nothing holding that: removing the check left every integration case green. The reason is structural rather than an oversight in the cases. Those are driven by role-based API keys, and a key's grant list cannot contain a permission for an entity that was never registered, so the decision refuses it anyway and the registry check is invisible from there. The caller that reaches it is a privileged session, because `checkAccess` short-circuits on super-admin before it reads any rule and answers yes for a slug that exists nowhere. These cases drive that directly, and one of them asserts the decision is not even ASKED about an unregistered slug rather than merely answered no. The last case holds the two entry points against each other entity for entity, which is the property the module exists for: a caller admitted by the list and refused by the single answer is the drift they share a module to prevent.
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. |
|
Warning Review limit reachedNext included review available in 48 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (19)
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. Comment |
Collaborator
Author
|
@codex review |
Contributor
@nextlyhq/adapter-drizzle
@nextlyhq/adapter-mysql
@nextlyhq/adapter-postgres
@nextlyhq/adapter-sqlite
@nextlyhq/admin
@nextlyhq/admin-css
@nextlyhq/blocks-engine
@nextlyhq/blocks-react
@nextlyhq/builder
create-nextly-app
@nextlyhq/eslint-plugin
nextly
@nextlyhq/plugin-form-builder
@nextlyhq/plugin-mcp
@nextlyhq/plugin-page-builder
@nextlyhq/plugin-sdk
@nextlyhq/plugin-seo
@nextlyhq/storage-s3
@nextlyhq/storage-uploadthing
@nextlyhq/storage-vercel-blob
@nextlyhq/ui
commit: |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eb69ae63e3
ℹ️ 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".
github-actions
Bot
added
scope: core
…read content Review round two on the schema tools. Most of it traces to one habit. `get_single_schema` read the singles facade's result as `items` when it answers with `data`, so every successful call threw. A locally redeclared interface hid the mismatch from the compiler, and no test caught it because every single case exercised the refusal path, which returns before the registry is read. Both tools now import the facade's own types, and a case drives a real single's schema end to end. Removing that redeclaration exposed two more defects it had been hiding. The read passed a user context built from `ctx.user` alone, which carries no roles and no key scope, and its context parameter is documented as unused. The registry read is not access-controlled and the authorization that decides already ran above it, so it now passes an empty context rather than a fabricated identity that would imply the call is gated by one. Both tools serve clients that read only `content`. A client on a 2025 revision does not understand structured output, and the library appends a text rendering only when `structuredContent` is a non-object value, so an object-shaped result reached those clients as a success with an empty body. `get_initial_context` has the same defect and is already released; it now carries its data as a second block, after the instructions block, which stays a constant so a hostile entity name still cannot reach the sentence telling the agent how to behave. The kind is checked in both directions. A single's slug through the collection tool surfaced a registry not-found instead of the uniform refusal, and that difference is measurable by an unauthorized caller. A container field's children survive the projection, and type-specific declaration travels with them, so a group or a repeater no longer arrives as a field of no particular shape. `readableContentKind` answers the access question and the kind question in one registry pass, and `canReadContent` is derived from it rather than asking again. The stability ledger now lists these exports. It listed neither `readableContent` nor `routePathIsLiteral`, both already published, and that ledger treats every unlisted export as internal.
Collaborator
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e28c1301b7
ℹ️ 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".
…s own Review round three. The first two findings are consequences of round two's fix, which started forwarding type-specific declaration and so made its shape matter. `options` is not one shape. A select or radio declares an ARRAY of label/value pairs; the legacy registry definition uses the same key for an object bag carrying a number's format or a relation's target. The output schema admitted only the object, and the server validates a tool result against that schema, so forwarding a select field's options converted an otherwise successful lookup into a validation failure. Both shapes are admitted now, and a select field is driven end to end. A relationship's `relationTo` and `hasMany` are top-level members rather than entries in that bag, so a projection copying only the bag returned a relationship's name and type and nothing a client could act on: one id, an array of ids and a polymorphic reference are told apart by exactly those two. `get_single_schema` asks the registry for the one slug it wants, through the slug allowlist the list options already carry. The registry deserializes each record it returns, so the unfiltered form materialized the whole registry to answer about one. Asserted on the arguments, because listing everything and searching in memory returns the same schema. The suites and `registerSchemaTools` are split so no function here is large or complex enough to gate. That was a real finding rather than a metric to dodge: these files are new, so the health baseline carries no entry for them and every finding in them counts as added. The cause was repeating `body.result?.structuredContent?.fields ?? []` per assertion, where each optional chain is a branch; the readings now live in one helper apiece.
Collaborator
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 00913bf6ec
ℹ️ 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".
Several surfaces have to describe a field to a consumer outside core, and each was projecting the declaration member by member. A projection written that way is a list somebody has to remember to extend, and three declaration keys went missing from one in three consecutive reviews: a select's options, a relationship's target and cardinality, and the slugs a field group points at. Each was found by a person reading the code rather than by anything going red. A fourth was never reported. The Builder spells a select's choices `fieldOptions` and code-first spells them `options`, and only the second was being copied, so a Builder-authored select returned no choices at all while every code-first fixture looked correct. `declaredShape` is that projection, once, with the set of keys it carries held as data rather than as control flow. The allowlist is deliberate. A denylist cannot lose a declaration key, which is the defect above, but it fails the other way: a key added to a stored declaration would reach every consumer the day it is written, and one of these consumers describes an install to a caller holding a narrowly scoped credential. Losing a key is a defect somebody reports; publishing one nobody classified is a disclosure nobody sees. The recurrence is closed by the guard instead. `declared-shape.test.ts` reads the manifest field schema every stored declaration is validated against and asserts the classification is TOTAL over it, so a key added there belongs to exactly one of `SCHEMA_FIELD_KEYS` or `WITHHELD_FIELD_KEYS` and a key in neither fails the build. The question review kept having to ask is asked by a test, on the commit that adds the key. A function value never travels, whatever key it sits on: `defaultValue` is a plain value on a stored declaration and a callable on a code-first one, so the key is worth publishing and that form of it serializes to nothing. `fields` survives only where the type is a container and the value really is an array, which is the rule the Singles facade already applied; a field group stays a leaf, because its children belong to the group's own declaration.
Round four, and the same finding shape for the third time, so this changes the design rather than adding another member to it. Rounds two, three and four each reported that the field projection had dropped a declaration key: a select's options, then a relationship's target and cardinality, then the slugs a component field points at. A list of member names cannot notice a name missing from it, and three people reading carefully is not a control. A fourth key was missing and nobody reported it: the Builder writes a select's choices as `fieldOptions` where code-first writes `options`, so every Builder-authored select answered with no choices while each code-first fixture looked correct. The tools now forward core's `declaredShape` and hold no member list at all. The Singles facade published `name`, `type` and nested `fields` and nothing else, so no Single could answer with a select's choices or a relationship's target however this tool projected them. It reduces records through the same projection now, which fixes every consumer of `ctx.services.singles`. A relationship's target is authorized before it travels. A schema names other entities from inside itself, so forwarding the target of a field pointing at a collection the caller was refused tells them it exists, which is the enumeration the uniform refusal prevents, reached from another direction. A polymorphic target keeps the arms the caller may read; the key goes entirely when none survive, rather than staying as an empty array. Redaction asks whether a target is WITHHELD, not whether it is unreadable, and the difference was measured rather than assumed. `users`, `media`, `roles` and `permissions` are in neither content registry, so a rule keyed on readability strips the target from every upload field and every relationship to a system entity, for a super administrator included. `contentReadability` reports the registry fact and the access fact together so the two can be told apart, and `readableContentKind` is derived from it rather than asking again. `readableContentKind` takes a point lookup where it enumerated both registries. An agent inspecting the entities `get_initial_context` listed was scanning every registry row once per entity. The two readings are separate reads of one question, so an integration test holds them to the same answer against real registries, which a unit test cannot: mocking drives both from one fixture and removes the disagreement being guarded against.
…aled it Four comments explained why the field projection is shared by recounting how the omissions were found. A comment that names reviews dates itself, tells a reader nothing they can act on, and the convention check refuses it. Each now states the property instead. A projection spelled as a list of member names cannot notice a name missing from it, and the keys that go missing are the type-specific ones, because each belongs to a minority of field types and no fixture built from the others exercises it. The same shape hides a spelling split: a select declares its choices as `options` from code and `fieldOptions` from the Schema Builder, so a projection carrying one of the two is correct for every code-first fixture and returns nothing for a Builder-authored field. Behaviour is unchanged.
Collaborator
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ff117aa2c6
ℹ️ 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".
Two defects, and the first is one this change introduced a round earlier. The registry point lookup caught a failed read and returned false, folding "the lookup threw" into "there is no such entity". That is the right answer for an access decision, which refuses either way, and the wrong one for a DISCLOSURE decision, which reads absence as permission: a target that is not registered content is safe to name, so a schema describing a relationship would publish the slug of the very collection the caller was refused, and would do it exactly when the registry was already unwell. `registryRead` sits directly above and spends a paragraph explaining why a failure must not read as absence for the enumeration path; the point lookup beneath it collapsed the same distinction. One value cannot answer two questions whose safe directions are opposite, so the lookup is three-valued now. `present`, `absent` and `unknown` travel as far as the caller that has to choose: `ContentReadability` carries `known`, `readableContentKind` still refuses on unknown exactly as it refuses on absent, and the schema tools withhold a target whose existence was never established. The projection also dropped four constraints. A code-first field may write `minLength`, `maxLength`, `min` or `max` FLAT rather than nested under `validation`, and the type generator reads the flat spelling when it builds the entity's validation schema, so the tools described a field as unbounded while a write exceeding the bound was still rejected. The reader was not told and the write still failed. The guard that was supposed to make that impossible could not see it. It asserts the key classification is total over the manifest field schema, and the manifest nests these under `validation`, so a key that exists only in the flat code-first spelling was outside the domain it was total over. A guard total over one of two schemas is a guard with the other one's keys outside it. It now also asserts totality over the keys the generator consumes, read from that source rather than re-listed, so a constraint the API enforces cannot go undescribed. Both fixes are driven end to end. A case makes one target's registry lookup throw and asserts the target is not named, with a spy assertion so it cannot pass unless the failure actually occurred; another pins that the unknown state is reported separately from the unregistered one, since both refuse a read and only one of them means the entity does not exist.
…ives The three-valued registry lookup was driven only end to end, through the schema tools, and that suite resolves core from BUILT output. So the branch that distinguishes a failed lookup from an absent slug had no test at the layer it lives in, and any attempt to break it there would have measured a stale build rather than the code. Added beside the snapshot cases, which exist for the same distinction one level up: absence and "could not answer" look identical and callers treat them oppositely. Seven cases cover which registry holds a slug, the precedence when both would answer, a slug neither holds reported as KNOWN absent, a lookup that throws reported as unknown, and a service that fails to CONSTRUCT reported the same way, since `container.get` invokes the factory and a throw there is indistinguishable from an unregistered name unless it is asked for separately. The last case is the control. A registry that is simply not registered must still report absence, or a disclosure decision withholds forever on an ordinary install and the conservative direction becomes useless rather than safe.
Collaborator
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dd65a72070
ℹ️ 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".
Comment on lines
+161
to
+164
| const asCollection = await registryHolds<CollectionLookup>( | ||
| "collectionRegistryService", | ||
| registry => registry.getCollectionBySlug(slug) | ||
| ); |
There was a problem hiding this comment.
When a caller requests a forbidden slug, servable reaches this lookup before canReadEntity; getCollectionBySlug/getSingleBySlug select and deserialize the complete registry record, including its fields, so the unauthorized schema is still loaded and its size can affect refusal timing. Fresh evidence after the earlier ordering fix is that its spy watches the later collectionService.getCollection call, not these newly added pre-authorization registry reads. Check access first or add a slug-only presence lookup so a denial never materializes the schema.
AGENTS.md reference: AGENTS.md:L309-L312
Useful? React with 👍 / 👎.
Comment on lines
+159
to
+162
| return { | ||
| kind, | ||
| readable: await canReadEntity(slug, asReadAccess(caller)), | ||
| known, |
There was a problem hiding this comment.
When a session RBAC lookup rejects because the permission database or pool is unavailable, this direct await rejects too. The set path handles the same failed decision through Promise.allSettled and omits the entity, but both schema tools instead return an internal MCP error for a registered slug while an absent slug still gets the uniform refusal, making existence distinguishable during authorization degradation. Convert a rejected decision to readable: false while retaining the known registry kind.
AGENTS.md reference: AGENTS.md:L299-L302
Useful? React with 👍 / 👎.
Comment on lines
+117
to
+120
| "options", | ||
| "fieldOptions", | ||
| "defaultValue", | ||
| "default", |
There was a problem hiding this comment.
For a code-first json field, JSONFieldConfig.jsonSchema is serialized into the registry but is absent from this allowlist, so both new tools reduce the field to its name/type/default and omit the object properties, required keys, and constraints that actually describe its value. The totality checks do not catch this because jsonSchema appears in neither the manifest schema nor zod-generator.ts; classify it from the richer code-first field contract as well.
AGENTS.md reference: AGENTS.md:L299-L302
Useful? React with 👍 / 👎.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Phase 5.6 of the AI-native programme. Based on
main, not stacked.get_initial_context(5.5) says which entities an agent can work with. These two tools say what one of them looks like.Where the schema comes from
This is the open question the design round left for the build: where should introspection read, so it reports the same shape
generate-typesdoes rather than becoming a second answer?Answered by measurement: the registry.
syncCodeFirstCollectionswrites code-defined collections into the samedynamic_collectionstable the Schema Builder writes to, andgenerate-typesreads those records. So the registry is the merged view of both sources, and reading it means the tool cannot drift from the generated types.Reading the config object instead would have answered correctly for a code-first install, been blind to every collection an operator built in the admin, and looked right in both.
Authorization runs before the read, and has to
collections.getCollectiontakes a request context and does not use it. The registry read is deliberately not access-controlled, because the registry is how the system describes itself. The gate in the tool is therefore the only thing between a scoped key and the shape of an entity it was never granted, which makes its ORDERING a precondition rather than a filter applied to a result.The decision is core's
canReadContent, the same one that built the entity list in 5.5, so the two tools cannot disagree about what a caller may see.A refusal says nothing about existence
"You may not read this" and "no such entity" are one answer. A caller who can tell them apart maps an install's slugs by asking about guesses, which is disclosure by error message.
Two details worth a look
A field's name stays optional. Presentational field types carry none. Defaulting to an empty string would put a field in the answer that an agent could then try to read, so absence is reported as absence.
canReadContentrefuses an unregistered slug rather than judging it. A slug with no registry entry has no rule to decide against, and admitting what cannot be judged is the inversion the dashboard's readable resources were fixed to remove.Evidence
fallow auditall cleanThe middle one survived its first run, and the reason is worth stating. Nothing covered the unregistered-slug guard, and not because the cases were careless: they are driven by role-based API keys, and a key's grant list cannot contain a permission for an entity that was never registered, so the decision refuses it anyway and the registry check is invisible from there. The caller that reaches it is a privileged session, because
checkAccessshort-circuits on super-admin before reading any rule.packages/nextly/src/auth/__tests__/readable-content.test.tsdrives that directly, and asserts the decision is not even ASKED about an unregistered slug rather than merely answered no.One earlier mutation of mine also survived and should not have counted: it reworded both refusal messages identically, which is not a leak. Replaced with one that actually introduces the distinction, which the test catches.