Conversation
…-for-project-definition-with
🦋 Changeset detectedLatest commit: 880efec The changes in this PR will be included in the next version bump. This PR includes changesets to release 21 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (3)
📒 Files selected for processing (9)
📝 WalkthroughWalkthroughThis PR introduces a comprehensive entity service system for project-builder-lib, enabling read/write operations on typed entities within project definitions. It adds CLI commands (definition command with 10 subcommands) and 10 corresponding server actions, along with draft session management for staging changes before commit. Supporting utilities include schema traversal, entity navigation, and immutable updates. Dependencies are updated (immer 11.1.4) and test paths are migrated to new locations. Changes
Sequence DiagramssequenceDiagram
participant Client as CLI Client
participant Dev as Dev CLI
participant Action as Service Action
participant Context as Entity Service
participant Lib as Project Builder Lib
participant Def as Definition Container
Client->>Dev: definition list-entities
Dev->>Action: listEntitiesAction
Action->>Context: loadEntityServiceContext(project)
Context->>Lib: loadProjectDefinition()
Lib->>Def: fromSerializedConfig()
Def-->>Context: ProjectDefinitionContainer
Context->>Def: toEntityServiceContext()
Def-->>Context: EntityServiceContext
Action->>Lib: listEntities(entityTypeName, context)
Lib-->>Action: EntityStub[]
Action-->>Dev: CLI formatted output
Dev-->>Client: Display entities
sequenceDiagram
participant Client as CLI Client
participant Dev as Dev CLI
participant Action as Service Action
participant Draft as Draft Session
participant Def as Definition Validator
participant Disk as File System
participant Lib as Project Builder Lib
Client->>Dev: definition stage-create-entity
Dev->>Action: stageCreateEntityAction
Action->>Draft: getOrCreateDraftSession(project)
Draft->>Disk: Load or create draft
Draft-->>Action: DraftSessionContext
Action->>Lib: createEntity(data, context)
Lib-->>Action: New entity in definition
Action->>Def: validateDraftDefinition()
Def->>Lib: collectDefinitionIssues()
Lib-->>Def: Issues found/errors
alt Validation Errors
Def-->>Action: Error
Action-->>Client: Staging blocked
else Valid
Def-->>Action: Warnings only
Action->>Draft: saveDraftSession()
Draft->>Disk: Write draft files
Action-->>Client: Staged with warnings
end
sequenceDiagram
participant Client as CLI Client
participant Dev as Dev CLI
participant ShowDraft as show-draft Action
participant Draft as Draft Session
participant Lib as Project Builder Lib
participant Diff as Diff Engine
Client->>Dev: definition show-draft
Dev->>ShowDraft: showDraftAction
ShowDraft->>Draft: loadDraftSession(project)
Draft-->>ShowDraft: DraftSession
ShowDraft->>Lib: loadEntityServiceContext(project)
Lib-->>ShowDraft: Current EntityServiceContext
ShowDraft->>Diff: diffSerializedDefinitions(current, draft)
Diff-->>ShowDraft: DefinitionDiff with changes
ShowDraft->>ShowDraft: Format changes (JSON, JSON Patch)
ShowDraft-->>Dev: Formatted diff output
Dev-->>Client: Display draft changes
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly Related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (11)
packages/project-builder-server/src/actions/definition/discard-draft.action.ts (1)
8-10: Consider adding minimum length validation for the project input.Using
z.string().min(1)would reject empty strings at schema validation, providing a clearer error message to the user rather than failing downstream ingetProjectByNameOrId.💡 Suggested improvement
const discardDraftInputSchema = z.object({ - project: z.string().describe('The name or ID of the project.'), + project: z.string().min(1).describe('The name or ID of the project.'), });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/project-builder-server/src/actions/definition/discard-draft.action.ts` around lines 8 - 10, Update the Zod schema for discardDraftInputSchema to reject empty strings by adding a minimum length check: change the project field from z.string() to z.string().min(1) (and optionally include a .describe() or .message()) so validation fails fast and clearer errors are returned before reaching getProjectByNameOrId; ensure the reference name discardDraftInputSchema is used and tests/consumers expecting validation messages are adjusted if needed.packages/project-builder-server/src/actions/definition/draft-session.ts (1)
62-68: Consider resilience for partially corrupted draft state.If
draft-session.jsonexists butdraft-definition.jsonis missing (e.g., due to a previous write failure),loadDraftSessionwill throw an unhandled error fromreadFile. Consider checking both files exist before reading, or catching the error to treat it as a corrupted draft that should be discarded.🛠️ Suggested fix
export async function loadDraftSession( projectDirectory: string, ): Promise<DraftSession | null> { const sessionPath = getDraftSessionPath(projectDirectory); - if (!(await fileExists(sessionPath))) { + const definitionPath = getDraftDefinitionPath(projectDirectory); + if (!(await fileExists(sessionPath)) || !(await fileExists(definitionPath))) { return null; } const [sessionContents, definitionContents] = await Promise.all([ readFile(sessionPath, 'utf-8'), - readFile(getDraftDefinitionPath(projectDirectory), 'utf-8'), + readFile(definitionPath, 'utf-8'), ]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/project-builder-server/src/actions/definition/draft-session.ts` around lines 62 - 68, loadDraftSession currently checks only draft-session.json existence then calls readFile on both files; if draft-definition.json is missing a readFile will throw. Modify loadDraftSession to either 1) check existence of both files (use fileExists on sessionPath and on getDraftDefinitionPath(projectDirectory)) and return null if either is missing before calling readFile, or 2) wrap the Promise.all([...readFile...]) in a try/catch and treat any read/parse error as a corrupted draft (return null and optionally remove the partial files); reference loadDraftSession, sessionPath, getDraftDefinitionPath, fileExists, and readFile when making the change.packages/project-builder-server/src/actions/definition/stage-create-entity.action.ts (1)
28-34: Return the generated entity id fromstage-create-entity.This action stages a newly generated entity but only returns a generic message. Exposing the created
entityIdhere would make follow-upstage-update-entity/stage-delete-entitycalls against draft-only entities scriptable without forcing callers to inspectshow-draft.Also applies to: 47-54, 70-73
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/project-builder-server/src/actions/definition/stage-create-entity.action.ts` around lines 28 - 34, Update the staged-entity action to return the created entity's id: add an entityId field (string, e.g. z.string().describe('ID of the staged entity')) to stageCreateEntityOutputSchema and ensure the action handler that calls the entity creation returns { message, entityId, issues? } (use the actual id returned by the create/storage call inside the function that performs staging, e.g., where createEntity/insertDraftEntity is invoked). Also update any other places that construct this action's response (the code paths referenced around the other return blocks) so they include the same entityId value or null/undefined consistently when no id was created. Ensure all references use the exact symbol stageCreateEntityOutputSchema and the action handler that builds the response so the schema and runtime result stay in sync.packages/project-builder-server/src/actions/definition/get-entity-schema.action.ts (1)
45-48: Point the recovery hint atlist-entity-types.This PR already adds a dedicated discovery action, so using it here keeps the recovery path aligned with the actual API instead of depending on wildcard behavior in
list-entities.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/project-builder-server/src/actions/definition/get-entity-schema.action.ts` around lines 45 - 48, The error message thrown in get-entity-schema.action.ts when metadata is missing references the wrong recovery hint; update the thrown Error where it checks if (!metadata) to suggest using the new discovery action name "list-entity-types" instead of "list-entities" with wildcard. Locate the throw that builds the message with input.entityTypeName and replace the recovery hint text to point developers to use list-entity-types for discovering available types while keeping the existing descriptive context.packages/project-builder-server/src/actions/definition/definition-actions.int.test.ts (2)
30-31: Mocknode:fsalongsidenode:fs/promises.This suite is exercising draft-file persistence through memfs. Leaving the sync fs module live makes it easy for future implementation changes to bypass the virtual filesystem and hit the host filesystem instead.
Based on learnings, in the project-builder-server test suite, Vitest automocks for `node:fs` and `node:fs/promises` are already configured to use memfs without needing explicit implementation replacement in each test file.📦 Small test-hardening diff
vi.mock('./load-entity-service-context.js'); +vi.mock('node:fs'); vi.mock('node:fs/promises');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/project-builder-server/src/actions/definition/definition-actions.int.test.ts` around lines 30 - 31, The test currently mocks './load-entity-service-context.js' and 'node:fs/promises' but leaves the synchronous 'node:fs' module live, which can bypass memfs; add a mock for 'node:fs' (e.g., call vi.mock('node:fs')) alongside the existing vi.mock('node:fs/promises') in the same file so all fs APIs used by the test run against the virtual filesystem; update the test header where vi.mock('./load-entity-service-context.js') and vi.mock('node:fs/promises') are declared to include vi.mock('node:fs').
215-304: Add one end-to-end draft lifecycle test.This suite proves the stage-* writes, but the new public workflow also depends on
show-draft,commit-draft, anddiscard-draft. One happy-path test through that flow would catch regressions in the.build/draft-*.jsoncontract.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/project-builder-server/src/actions/definition/definition-actions.int.test.ts` around lines 215 - 304, Add a single end-to-end test that exercises the full draft lifecycle: call stageCreateEntityAction (or reuse existing stage-create-entity test setup) then invoke the show-draft action (showDraftAction or the "show-draft" CLI action) to assert the draft files contain the staged change, call commit-draft (commitDraftAction / "commit-draft") and assert the committed definition on disk includes the new feature and that draft-*.json files are removed/cleared, and finally call discard-draft (discardDraftAction / "discard-draft") or assert no leftover drafts; reference the existing test helpers invokeServiceActionAsCli, PROJECT_DIR/.build/draft-session.json and draft-definition.json, and the entity name 'payments' to locate where to add this new end-to-end test.packages/project-builder-lib/src/tools/entity-service/entity-write.ts (2)
85-91: Validation check doesn't use the retrieved value.The code retrieves
targetand validates it's a plain object, but then replaces it entirely withupdatedEntityat line 108. The validation is still useful as a defensive check, but consider adding a comment to clarify the intent, or removing the unusedisPlainObjectcheck since you're replacing the entire object anyway.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/project-builder-lib/src/tools/entity-service/entity-write.ts` around lines 85 - 91, The isPlainObject check currently validates the value returned by get(draft, entity.path) but that retrieved value is never used because you later replace the node with updatedEntity; either remove the redundant isPlainObject check or convert it into an explicit defensive assertion/comment that clarifies intent (e.g., "ensure existing node is an object before replacing") and keep it if you want to guard against replacing non-object paths; reference the get(draft, entity.path) call, the isPlainObject check, and the subsequent replacement with updatedEntity and update the code comment or remove the check accordingly so the intent matches the behavior.
93-100: Consider extracting shared path resolution logic.Both
updateEntityanddeleteEntitycontain identical logic to extractparentPathandentityIndexfromentity.path. This could be extracted into a small helper function for clarity and DRY.♻️ Optional refactor
function extractParentArrayInfo(entityPath: (string | number)[]): { parentPath: (string | number)[]; entityIndex: number; } { const parentPath = entityPath.slice(0, -1); const entityIndex = entityPath.at(-1); if (typeof entityIndex !== 'number') { throw new TypeError( `Expected numeric index at end of entity path "${entityPath.join('.')}"`, ); } return { parentPath, entityIndex }; }Also applies to: 139-146
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/project-builder-lib/src/tools/entity-service/entity-write.ts` around lines 93 - 100, Extract the duplicated path-resolution logic in updateEntity and deleteEntity into a small helper like extractParentArrayInfo that accepts entity.path (or entityPath: (string|number)[]) and returns { parentPath, entityIndex: number }; inside it call slice(0,-1), use .at(-1) and throw the existing TypeError if entityIndex is not a number, then replace the inlined logic in both updateEntity and deleteEntity to call this helper and use its returned parentPath and entityIndex values to keep behavior identical and DRY.packages/project-builder-server/src/actions/definition/commit-draft.action.ts (1)
25-31: Output schemaissuesfield is never populated.The
commitDraftOutputSchemadeclares an optionalissuesarray (lines 27-30), but when validation issues occur, they are thrown as an error (lines 79-84) rather than returned. The successful return (lines 96-98) never includesissues. Either remove the unused field from the schema or return issues as data when appropriate.🔧 Proposed fix: remove unused field
const commitDraftOutputSchema = z.object({ message: z.string().describe('A summary of the commit result.'), - issues: z - .array(definitionIssueSchema) - .optional() - .describe('Definition issues that blocked the commit.'), });Also applies to: 79-84, 96-98
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/project-builder-server/src/actions/definition/commit-draft.action.ts` around lines 25 - 31, The output schema commitDraftOutputSchema declares an optional issues array but the commit-draft logic currently throws validation errors instead of returning them and the success return never includes issues; update the commitDraft action so that when validation or definition issues are detected (where errors are currently thrown around the validation block) you transform those errors into an array matching definitionIssueSchema and return them as the issues property on the successful response object (e.g., return { message: string, issues }), or alternatively remove the unused issues field from commitDraftOutputSchema if you prefer not to return issues; locate the throw path (currently lines around the validation/error handling) and the successful return (currently the success return block) and modify one of those spots to either populate issues or delete the schema field accordingly.packages/project-builder-lib/src/tools/entity-service/entity-read.ts (2)
67-76: JSDoc parameter documentation is also outdated forgetEntity.Same issue as above - the documented parameters don't match the actual function signature.
📝 Proposed fix
/** * Gets a single entity by ID, returning its full serialized (name-based) data. * - * `@param` container - The project definition container - * `@param` entityTypeMap - The entity type map built from the schema - * `@param` serializedDef - The serialized project definition (with names) - * `@param` entityTypeName - The entity type to get - * `@param` entityId - The entity ID + * `@param` entityId - The entity ID to look up + * `@param` context - The entity service context * `@returns` The serialized entity data, or undefined if not found */🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/project-builder-lib/src/tools/entity-service/entity-read.ts` around lines 67 - 76, The JSDoc for getEntity is out of sync with its actual signature—update the comment block above the getEntity function to exactly match the real parameter names and return type used in the implementation (e.g., ensure parameters listed match the function's signature such as container, entityTypeMap, serializedDef, entityTypeName, entityId or whatever the current parameter names are in getEntity) and adjust the `@returns` description to reflect the actual returned type (serialized entity or undefined); keep descriptions brief and accurate and remove any stale params that no longer exist.
23-34: JSDoc parameter documentation is outdated.The JSDoc lists individual parameters (
container,entityTypeMap,serializedDef, etc.) but the actual function signature uses a destructured input object and acontextparameter. Update the documentation to match the actual API.📝 Proposed fix
/** * Lists all entities of a given type, returning lightweight stubs. * * For nested entity types, `parentEntityId` is required to scope the listing * to entities within a specific parent. * - * `@param` container - The project definition container - * `@param` entityTypeMap - The entity type map built from the schema - * `@param` entityTypeName - The entity type to list - * `@param` parentEntityId - Required for nested entity types + * `@param` input - The input containing entityTypeName and optional parentEntityId + * `@param` context - The entity service context * `@returns` Array of entity stubs with id, name, and type */🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/project-builder-lib/src/tools/entity-service/entity-read.ts` around lines 23 - 34, The JSDoc is out of sync with the actual function signature in entity-read.ts: update the comment to document the destructured input object and the separate context parameter instead of individual params like `container` and `entityTypeMap`; replace the `@param` entries with a single `@param` input that lists the destructured fields (e.g. container, entityTypeMap, serializedDef, entityTypeName, parentEntityId) and add `@param` context describing its shape/purpose, and ensure the `@returns` description matches the actual return value of the function (entity stubs with id, name, type). Reference the function implemented in entity-read.ts and adjust the JSDoc tags to match its destructured input and context parameters.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.agents/testing.md:
- Around line 106-107: Update the reference table to include the new exported
helper createTestEntityServiceContext: add a new table entry or extend the
existing row listing project-builder-lib exports to include
`createTestEntityServiceContext` alongside `createTestProjectDefinition`,
`createTestProjectDefinitionContainer`, and the other helpers so the new
entity-service context factory is discoverable.
In `@packages/project-builder-dev/src/commands/definition.ts`:
- Around line 150-153: The JSON.parse of entityDataJson in the stage-update
command is unguarded; wrap the parse in a try/catch and surface a clear error
like the existing pattern: attempt to parse entityDataJson into entityData
(Record<string, unknown>) and on failure throw or log a descriptive error that
includes the invalid JSON and context ("stage-update" or the command name).
Update the code around entityData and entityDataJson to mirror the same error
handling used elsewhere (try { const entityData = JSON.parse(entityDataJson) as
Record<string, unknown>; } catch (err) { throw new Error(`Invalid JSON for
stage-update entityData: ${String(err)}`); }) so callers get actionable
feedback.
- Around line 120-123: Wrap the JSON.parse call that constructs entityData in a
try-catch to handle malformed JSON: catch SyntaxError (or general errors) from
JSON.parse(entityDataJson) and surface a clear, user-friendly error message
(including the original error message and optionally the offending input)
instead of letting the raw exception propagate; update the code around the
entityData variable creation so functions/methods that rely on entityData (the
JSON.parse(entityDataJson) expression) either exit gracefully or rethrow a more
descriptive error after logging.
In `@packages/project-builder-lib/package.json`:
- Line 60: Immer 11's default "loose iteration" can change how produce() handles
Symbol keys and non-enumerable properties; review the uses of produce() in
entity-write.ts (the produce calls at the locations around lines where
create/update/delete are implemented) and either confirm that your definition
objects contain only plain enumerable properties or add a call to Immer's
setUseStrictIteration(true) immediately after importing Immer to restore the
previous strict behavior so future additions of Symbols/non-enumerable props
won't be skipped.
In `@packages/project-builder-lib/src/parser/walk-schema-structure.ts`:
- Around line 145-151: The code currently bypasses visiting the
discriminated-union node by calling walkDiscriminatedUnionArrayBranches
directly; instead, make the visitor see the element/union node first (so
wrappers/metadata like withEnt are preserved) by invoking the normal visit for
the element/union (using the same visitor entry that would handle a
discriminated-union node for unwrappedElement) and then, when recursing into its
branch children, call walkDiscriminatedUnionArrayBranches to descend into
branches while appending a 'discriminated-union-array' path element; apply the
same change to the analogous block around the 209-237 range so
elementChildren.kind === 'discriminated-union' always results in a normal visit
of children.elementSchema/discriminated-union node followed by specialized
branch descent.
In `@packages/project-builder-lib/src/parser/walk-schema-structure.unit.test.ts`:
- Around line 349-360: Replace the acyclic test schema with a truly
self-referential Zod schema using z.lazy so the visited-guard and the
visited.delete cleanup logic in walkSchemaStructure are exercised; specifically,
update the test that uses makeRecordingVisitor and walkSchemaStructure to create
a schema like a z.object({ nested: z.lazy(() => selfSchema) }) where selfSchema
references itself (via a const variable) and then call
walkSchemaStructure(selfSchema, [visitor]) and assert visitor.calls.length > 0
so the traversal completes and the backtrack delete path is executed.
In `@packages/project-builder-lib/src/tools/entity-service/entity-navigation.ts`:
- Around line 97-123: Add validation for parentEntityId/type before using
lookupEntity: if metadata.parentEntityTypeName is set, ensure parentEntityId is
provided and that lookupEntity(parentEntityId) returns an entity whose type
matches metadata.parentEntityTypeName (throw a descriptive Error mentioning
entityTypeName and parentEntityId on mismatch); conversely, if
metadata.parentEntityTypeName is not set but a parentEntityId was passed, throw
an Error rejecting extraneous parentEntityId to avoid silent ignores. Update the
checks around metadata.parentEntityTypeName, parentEntityId, and lookupEntity to
enforce these guards before assigning startPath or dereferencing
serializedDefinition.
In
`@packages/project-builder-server/src/actions/definition/list-entities.action.ts`:
- Around line 8-21: The schema listEntitiesInputSchema needs a refinement that
enforces parentEntityId when a nested entity type is requested: add a
.superRefine((data, ctx) => { if (isNestedCondition(data.entityTypeName) &&
!data.parentEntityId) ctx.addIssue({ code: z.ZodIssueCode.custom, message:
'parentEntityId is required for nested entity types', path: ['parentEntityId']
}); }) to the z.object; implement isNestedCondition either inline (e.g., check
for known nested names like 'model-scalar-field' or a pattern such as
entityTypeName.includes('-')) or by calling an existing helper, and keep
listEntities(...) behavior unchanged so callers get a validation error from the
schema instead of runtime rejection.
In `@packages/project-builder-server/src/actions/definition/show-draft.action.ts`:
- Around line 98-109: The current diff uses the live on-disk definition (via
loadEntityServiceContext -> container.toEntityServiceContext()) when calling
diffSerializedDefinitions against session.draftDefinition, which mixes unrelated
edits; change show-draft to compare session.draftDefinition against the draft’s
recorded base instead: persist the original serialized definition (e.g.,
session.baseSerializedDefinition or session.baseHash) when creating the draft
and use that value in diffSerializedDefinitions instead of
currentEntityContext.serializedDefinition, and if that persisted base is missing
or its hash mismatches the current disk definition, surface an “out-of-date
draft” state/error rather than silently diffing against the live file.
In
`@packages/project-builder-server/src/actions/definition/stage-update-entity.action.ts`:
- Around line 44-49: The update flow can accidentally change an entity's
identity when input.entityData omits id because assignEntityIds will generate a
new id; in the updateEntity path (called from stage-update-entity.action.ts)
either enforce id presence in the input schema or, simpler, validate and correct
inside updateEntity: if input.entityData.id exists ensure it equals
input.entityId (throw on mismatch), and if it is absent, set input.entityData.id
= input.entityId before calling assignEntityIds/update logic so the original
entity id is preserved; reference updateEntity and assignEntityIds when applying
this change.
In `@packages/project-builder-server/src/actions/types.ts`:
- Around line 26-27: The sessionId field is optional in the exported type but
your docstring promises a "default" fallback, so normalize it at the boundary
and make the handler-facing field required: ensure wherever the action/context
is constructed you replace undefined with "default" and change the exported
type's sessionId?: string to sessionId: string so downstream handlers always
receive a concrete value; alternatively centralize access through a single
helper (e.g., getSessionId(context)) that returns context.sessionId ?? "default"
and update handlers to use that helper so the invariant is always enforced.
---
Nitpick comments:
In `@packages/project-builder-lib/src/tools/entity-service/entity-read.ts`:
- Around line 67-76: The JSDoc for getEntity is out of sync with its actual
signature—update the comment block above the getEntity function to exactly match
the real parameter names and return type used in the implementation (e.g.,
ensure parameters listed match the function's signature such as container,
entityTypeMap, serializedDef, entityTypeName, entityId or whatever the current
parameter names are in getEntity) and adjust the `@returns` description to reflect
the actual returned type (serialized entity or undefined); keep descriptions
brief and accurate and remove any stale params that no longer exist.
- Around line 23-34: The JSDoc is out of sync with the actual function signature
in entity-read.ts: update the comment to document the destructured input object
and the separate context parameter instead of individual params like `container`
and `entityTypeMap`; replace the `@param` entries with a single `@param` input that
lists the destructured fields (e.g. container, entityTypeMap, serializedDef,
entityTypeName, parentEntityId) and add `@param` context describing its
shape/purpose, and ensure the `@returns` description matches the actual return
value of the function (entity stubs with id, name, type). Reference the function
implemented in entity-read.ts and adjust the JSDoc tags to match its
destructured input and context parameters.
In `@packages/project-builder-lib/src/tools/entity-service/entity-write.ts`:
- Around line 85-91: The isPlainObject check currently validates the value
returned by get(draft, entity.path) but that retrieved value is never used
because you later replace the node with updatedEntity; either remove the
redundant isPlainObject check or convert it into an explicit defensive
assertion/comment that clarifies intent (e.g., "ensure existing node is an
object before replacing") and keep it if you want to guard against replacing
non-object paths; reference the get(draft, entity.path) call, the isPlainObject
check, and the subsequent replacement with updatedEntity and update the code
comment or remove the check accordingly so the intent matches the behavior.
- Around line 93-100: Extract the duplicated path-resolution logic in
updateEntity and deleteEntity into a small helper like extractParentArrayInfo
that accepts entity.path (or entityPath: (string|number)[]) and returns {
parentPath, entityIndex: number }; inside it call slice(0,-1), use .at(-1) and
throw the existing TypeError if entityIndex is not a number, then replace the
inlined logic in both updateEntity and deleteEntity to call this helper and use
its returned parentPath and entityIndex values to keep behavior identical and
DRY.
In
`@packages/project-builder-server/src/actions/definition/commit-draft.action.ts`:
- Around line 25-31: The output schema commitDraftOutputSchema declares an
optional issues array but the commit-draft logic currently throws validation
errors instead of returning them and the success return never includes issues;
update the commitDraft action so that when validation or definition issues are
detected (where errors are currently thrown around the validation block) you
transform those errors into an array matching definitionIssueSchema and return
them as the issues property on the successful response object (e.g., return {
message: string, issues }), or alternatively remove the unused issues field from
commitDraftOutputSchema if you prefer not to return issues; locate the throw
path (currently lines around the validation/error handling) and the successful
return (currently the success return block) and modify one of those spots to
either populate issues or delete the schema field accordingly.
In
`@packages/project-builder-server/src/actions/definition/definition-actions.int.test.ts`:
- Around line 30-31: The test currently mocks './load-entity-service-context.js'
and 'node:fs/promises' but leaves the synchronous 'node:fs' module live, which
can bypass memfs; add a mock for 'node:fs' (e.g., call vi.mock('node:fs'))
alongside the existing vi.mock('node:fs/promises') in the same file so all fs
APIs used by the test run against the virtual filesystem; update the test header
where vi.mock('./load-entity-service-context.js') and
vi.mock('node:fs/promises') are declared to include vi.mock('node:fs').
- Around line 215-304: Add a single end-to-end test that exercises the full
draft lifecycle: call stageCreateEntityAction (or reuse existing
stage-create-entity test setup) then invoke the show-draft action
(showDraftAction or the "show-draft" CLI action) to assert the draft files
contain the staged change, call commit-draft (commitDraftAction /
"commit-draft") and assert the committed definition on disk includes the new
feature and that draft-*.json files are removed/cleared, and finally call
discard-draft (discardDraftAction / "discard-draft") or assert no leftover
drafts; reference the existing test helpers invokeServiceActionAsCli,
PROJECT_DIR/.build/draft-session.json and draft-definition.json, and the entity
name 'payments' to locate where to add this new end-to-end test.
In
`@packages/project-builder-server/src/actions/definition/discard-draft.action.ts`:
- Around line 8-10: Update the Zod schema for discardDraftInputSchema to reject
empty strings by adding a minimum length check: change the project field from
z.string() to z.string().min(1) (and optionally include a .describe() or
.message()) so validation fails fast and clearer errors are returned before
reaching getProjectByNameOrId; ensure the reference name discardDraftInputSchema
is used and tests/consumers expecting validation messages are adjusted if
needed.
In `@packages/project-builder-server/src/actions/definition/draft-session.ts`:
- Around line 62-68: loadDraftSession currently checks only draft-session.json
existence then calls readFile on both files; if draft-definition.json is missing
a readFile will throw. Modify loadDraftSession to either 1) check existence of
both files (use fileExists on sessionPath and on
getDraftDefinitionPath(projectDirectory)) and return null if either is missing
before calling readFile, or 2) wrap the Promise.all([...readFile...]) in a
try/catch and treat any read/parse error as a corrupted draft (return null and
optionally remove the partial files); reference loadDraftSession, sessionPath,
getDraftDefinitionPath, fileExists, and readFile when making the change.
In
`@packages/project-builder-server/src/actions/definition/get-entity-schema.action.ts`:
- Around line 45-48: The error message thrown in get-entity-schema.action.ts
when metadata is missing references the wrong recovery hint; update the thrown
Error where it checks if (!metadata) to suggest using the new discovery action
name "list-entity-types" instead of "list-entities" with wildcard. Locate the
throw that builds the message with input.entityTypeName and replace the recovery
hint text to point developers to use list-entity-types for discovering available
types while keeping the existing descriptive context.
In
`@packages/project-builder-server/src/actions/definition/stage-create-entity.action.ts`:
- Around line 28-34: Update the staged-entity action to return the created
entity's id: add an entityId field (string, e.g. z.string().describe('ID of the
staged entity')) to stageCreateEntityOutputSchema and ensure the action handler
that calls the entity creation returns { message, entityId, issues? } (use the
actual id returned by the create/storage call inside the function that performs
staging, e.g., where createEntity/insertDraftEntity is invoked). Also update any
other places that construct this action's response (the code paths referenced
around the other return blocks) so they include the same entityId value or
null/undefined consistently when no id was created. Ensure all references use
the exact symbol stageCreateEntityOutputSchema and the action handler that
builds the response so the schema and runtime result stay in sync.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: baafdb3a-865f-4950-a259-311b37131a9a
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (55)
.agents/code-style.md.agents/testing.md.changeset/add-immutable-set-utility.md.changeset/mcp-definition-entity-actions.mdpackages/project-builder-dev/src/commands/definition.tspackages/project-builder-dev/src/index.tspackages/project-builder-lib/package.jsonpackages/project-builder-lib/src/definition/index.tspackages/project-builder-lib/src/definition/plugins/plugin-utils.unit.test.tspackages/project-builder-lib/src/definition/project-definition-container.tspackages/project-builder-lib/src/parser/walk-schema-structure.tspackages/project-builder-lib/src/parser/walk-schema-structure.unit.test.tspackages/project-builder-lib/src/testing/index.tspackages/project-builder-lib/src/testing/project-definition-container.test-helper.tspackages/project-builder-lib/src/tools/assign-entity-ids.tspackages/project-builder-lib/src/tools/entity-service/entity-navigation.tspackages/project-builder-lib/src/tools/entity-service/entity-read.tspackages/project-builder-lib/src/tools/entity-service/entity-read.unit.test.tspackages/project-builder-lib/src/tools/entity-service/entity-type-map.tspackages/project-builder-lib/src/tools/entity-service/entity-write.tspackages/project-builder-lib/src/tools/entity-service/entity-write.unit.test.tspackages/project-builder-lib/src/tools/entity-service/index.tspackages/project-builder-lib/src/tools/entity-service/types.tspackages/project-builder-lib/src/tools/index.tspackages/project-builder-lib/src/tools/merge-schema/diff-definition.tspackages/project-builder-lib/src/tools/merge-schema/diff-definition.unit.test.tspackages/project-builder-lib/src/tools/merge-schema/index.tspackages/project-builder-lib/src/tools/merge-schema/merge-data-with-schema.tspackages/project-builder-lib/src/tools/merge-schema/merge-data-with-schema.unit.test.tspackages/project-builder-lib/src/tools/merge-schema/merge-definition.unit.test.tspackages/project-builder-lib/src/tools/merge-schema/walk-schema.unit.test.tspackages/project-builder-server/package.jsonpackages/project-builder-server/src/actions/__tests__/action-test-utils.tspackages/project-builder-server/src/actions/definition/commit-draft.action.tspackages/project-builder-server/src/actions/definition/definition-actions.int.test.tspackages/project-builder-server/src/actions/definition/discard-draft.action.tspackages/project-builder-server/src/actions/definition/draft-session.tspackages/project-builder-server/src/actions/definition/get-entity-schema.action.tspackages/project-builder-server/src/actions/definition/get-entity.action.tspackages/project-builder-server/src/actions/definition/index.tspackages/project-builder-server/src/actions/definition/list-entities.action.tspackages/project-builder-server/src/actions/definition/list-entity-types.action.tspackages/project-builder-server/src/actions/definition/load-entity-service-context.tspackages/project-builder-server/src/actions/definition/show-draft.action.tspackages/project-builder-server/src/actions/definition/stage-create-entity.action.tspackages/project-builder-server/src/actions/definition/stage-delete-entity.action.tspackages/project-builder-server/src/actions/definition/stage-update-entity.action.tspackages/project-builder-server/src/actions/definition/validate-draft.tspackages/project-builder-server/src/actions/index.tspackages/project-builder-server/src/actions/registry.tspackages/project-builder-server/src/actions/types.tspackages/project-builder-web/package.jsonpackages/utils/src/objects/immutable-set.tspackages/utils/src/objects/immutable-set.unit.test.tspackages/utils/src/objects/index.ts
💤 Files with no reviewable changes (1)
- packages/project-builder-lib/src/definition/index.ts
| "es-toolkit": "1.44.0", | ||
| "globby": "^14.0.2", | ||
| "immer": "10.1.1", | ||
| "immer": "~11.1.4", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Immer API call sites in project-builder-lib ==="
rg -n -C2 --glob 'packages/project-builder-lib/**/*.ts' --glob 'packages/project-builder-lib/**/*.tsx' '\b(produceWithPatches|applyPatches|enablePatches|createDraft|finishDraft|produce|current|original)\b' || echo "No matches found"
echo
echo "=== Patch / draft related tests in project-builder-lib ==="
rg -n -C2 --glob 'packages/project-builder-lib/**/*.test.ts' --glob 'packages/project-builder-lib/**/*.test.tsx' --glob 'packages/project-builder-lib/**/*.spec.ts' --glob 'packages/project-builder-lib/**/*.spec.tsx' '(patch|draft|immer|iteration)' -i || echo "No matches found"
echo
echo "=== Check if immer is actually imported/used ==="
rg -n "from.*immer|import.*immer|require.*immer" --glob 'packages/project-builder-lib/**/*.ts' --glob 'packages/project-builder-lib/**/*.tsx' || echo "No direct imports found"Repository: halfdomelabs/baseplate
Length of output: 50379
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Searching for Immer produce() usage in entity-write.ts ==="
sed -n '1,50p' packages/project-builder-lib/src/tools/entity-service/entity-write.ts | head -20
echo
echo "=== Full entity-write.ts for Immer usage context ==="
grep -n "produce" packages/project-builder-lib/src/tools/entity-service/entity-write.tsRepository: halfdomelabs/baseplate
Length of output: 1041
🌐 Web query:
Immer 11.0.0 changelog breaking changes produce API
💡 Result:
Immer 11.0.0 (released 2025-11-23) — changelog / breaking changes (esp. around produce)
Breaking change that can affect produce
- “Loose iteration” is enabled by default. This changes how Immer walks object properties during drafting/finalization/freezing/patch generation:
- Loose iteration = enumerable string keys only (better perf)
- Strict iteration = includes Symbol keys + non-enumerable properties (more complete)
You can switch back viasetUseStrictIteration(true). [1][2][3]
Practical impact on produce: if your state uses Symbol keys or non-enumerable properties and you expected Immer to copy/freeze/patch them, that behavior can change under the new default (they may be skipped unless you enable strict iteration). [2][3]
11.0.0 release notes (other items listed under “BREAKING CHANGES”, mostly internal/perf)
- Finalization rewritten to use a callback approach instead of recursive tree traversal (perf improvement). [1]
- Internal changes called out in the breaking list, including: simplifying iteration checks, allowing passing a known “type” to internal get/set helpers, converting an internal
assigned_structure toMap, and replacing recursive tree finalization with targeted callbacks. [1]
produce API surface
- The core
produce(base, recipe, ?patchListener)API is still the core API; the main behavioral change in 11.0.0 relevant toproduceis the iteration default described above, not a newproduce()signature. [2]
Sources:
[1] Immer v11.0.0 release notes / breaking changes list (GitHub release mirror) (newreleases.io)
[2] Immer official API docs (setUseStrictIteration: loose vs strict, and default) (immerjs.github.io)
[3] PR #1183 notes explicitly stating strictIteration: false as the breaking change for perf (“loose iteration”) (github.com)
Citations:
- 1: https://newreleases.io/project/github/immerjs/immer/release/v11.0.0
- 2: https://immerjs.github.io/immer/api/?utm_source=openai
- 3: Rewrite finalization system to use a callback approach instead of tree traversal immerjs/immer#1183
Immer 11.0.0 changes are low-risk for this code, but verification still recommended.
The package does use produce() in entity-write.ts (lines 39, 85, 148) for create/update/delete operations. Immer 11's main breaking change is "loose iteration" by default—skipping Symbol keys and non-enumerable properties during drafting and finalization. Since the codebase works with plain definition objects (no Symbol keys detected), the basic produce() calls should work without issues. However, if custom non-enumerable properties or Symbols are added to definition objects in the future, calling setUseStrictIteration(true) after the Immer import may be needed to restore the old behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/project-builder-lib/package.json` at line 60, Immer 11's default
"loose iteration" can change how produce() handles Symbol keys and
non-enumerable properties; review the uses of produce() in entity-write.ts (the
produce calls at the locations around lines where create/update/delete are
implemented) and either confirm that your definition objects contain only plain
enumerable properties or add a call to Immer's setUseStrictIteration(true)
immediately after importing Immer to restore the previous strict behavior so
future additions of Symbols/non-enumerable props won't be skipped.
Comment on lines
+145
to
+151
| if (elementChildren.kind === 'discriminated-union') { | ||
| walkDiscriminatedUnionArrayBranches( | ||
| unwrappedElement as ZodDiscriminatedUnion, | ||
| path, | ||
| visitors, | ||
| visited, | ||
| ); |
There was a problem hiding this comment.
Don't bypass the discriminated-union element schema here.
Line 145 jumps from the array node straight into each branch option, so visitors never see children.elementSchema or the discriminated-union node itself. That drops any element-level wrappers/metadata — for example z.discriminatedUnion(...).apply(withEnt(...)) — so the parent entity for DU-array schemas cannot be collected. Please preserve a normal visit for the element/union node and only specialize the child descent to append discriminated-union-array path elements.
Also applies to: 209-237
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/project-builder-lib/src/parser/walk-schema-structure.ts` around
lines 145 - 151, The code currently bypasses visiting the discriminated-union
node by calling walkDiscriminatedUnionArrayBranches directly; instead, make the
visitor see the element/union node first (so wrappers/metadata like withEnt are
preserved) by invoking the normal visit for the element/union (using the same
visitor entry that would handle a discriminated-union node for unwrappedElement)
and then, when recursing into its branch children, call
walkDiscriminatedUnionArrayBranches to descend into branches while appending a
'discriminated-union-array' path element; apply the same change to the analogous
block around the 209-237 range so elementChildren.kind === 'discriminated-union'
always results in a normal visit of children.elementSchema/discriminated-union
node followed by specialized branch descent.
Comment on lines
+97
to
+123
| if (metadata.parentEntityTypeName) { | ||
| // Nested entity: find the parent first | ||
| if (!parentEntityId) { | ||
| throw new Error( | ||
| `Entity type "${entityTypeName}" requires a parent entity ID (parent type: "${metadata.parentEntityTypeName}")`, | ||
| ); | ||
| } | ||
|
|
||
| const parentResult = lookupEntity(parentEntityId); | ||
| if (!parentResult) { | ||
| throw new Error( | ||
| `Parent entity "${parentEntityId}" not found for entity type "${entityTypeName}"`, | ||
| ); | ||
| } | ||
|
|
||
| startPath = parentResult.path; | ||
| } | ||
|
|
||
| const parentEntity = ( | ||
| startPath.length === 0 | ||
| ? serializedDefinition | ||
| : get(serializedDefinition, startPath) | ||
| ) as unknown; | ||
| if (!isPlainObject(parentEntity)) { | ||
| throw new Error( | ||
| `Parent entity at path "${startPath.join('.')}" is not a plain object`, | ||
| ); |
There was a problem hiding this comment.
Validate parentEntityId against the target type.
Right now any existing entity ID is accepted for nested types, and extra parentEntityId values are silently ignored for top-level types. That pushes bad caller input downstream into path-navigation errors, and it leaves room for wrong-scope writes if different parent types ever share a compatible shape.
🛠️ Suggested guardrails
if (metadata.parentEntityTypeName) {
// Nested entity: find the parent first
if (!parentEntityId) {
throw new Error(
`Entity type "${entityTypeName}" requires a parent entity ID (parent type: "${metadata.parentEntityTypeName}")`,
);
}
const parentResult = lookupEntity(parentEntityId);
if (!parentResult) {
throw new Error(
`Parent entity "${parentEntityId}" not found for entity type "${entityTypeName}"`,
);
}
+ if (parentResult.type !== metadata.parentEntityTypeName) {
+ throw new Error(
+ `Entity type "${entityTypeName}" requires parent type "${metadata.parentEntityTypeName}", got "${parentResult.type}"`,
+ );
+ }
startPath = parentResult.path;
+ } else if (parentEntityId) {
+ throw new Error(
+ `Entity type "${entityTypeName}" does not accept a parent entity ID`,
+ );
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/project-builder-lib/src/tools/entity-service/entity-navigation.ts`
around lines 97 - 123, Add validation for parentEntityId/type before using
lookupEntity: if metadata.parentEntityTypeName is set, ensure parentEntityId is
provided and that lookupEntity(parentEntityId) returns an entity whose type
matches metadata.parentEntityTypeName (throw a descriptive Error mentioning
entityTypeName and parentEntityId on mismatch); conversely, if
metadata.parentEntityTypeName is not set but a parentEntityId was passed, throw
an Error rejecting extraneous parentEntityId to avoid silent ignores. Update the
checks around metadata.parentEntityTypeName, parentEntityId, and lookupEntity to
enforce these guards before assigning startPath or dereferencing
serializedDefinition.
Comment on lines
+8
to
+21
| const listEntitiesInputSchema = z.object({ | ||
| project: z.string().describe('The name or ID of the project.'), | ||
| entityTypeName: z | ||
| .string() | ||
| .describe( | ||
| 'The entity type to list (e.g., "feature", "model", "model-scalar-field").', | ||
| ), | ||
| parentEntityId: z | ||
| .string() | ||
| .optional() | ||
| .describe( | ||
| 'Required for nested entity types. The ID of the parent entity to scope the listing.', | ||
| ), | ||
| }); |
There was a problem hiding this comment.
Encode the parentEntityId precondition in the schema.
The description says this field is required for nested entity types, but the action currently accepts requests without it in all cases. Adding a refinement here would give CLI/MCP callers a clear validation error instead of relying on listEntities(...) to reject it later.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@packages/project-builder-server/src/actions/definition/list-entities.action.ts`
around lines 8 - 21, The schema listEntitiesInputSchema needs a refinement that
enforces parentEntityId when a nested entity type is requested: add a
.superRefine((data, ctx) => { if (isNestedCondition(data.entityTypeName) &&
!data.parentEntityId) ctx.addIssue({ code: z.ZodIssueCode.custom, message:
'parentEntityId is required for nested entity types', path: ['parentEntityId']
}); }) to the z.object; implement isNestedCondition either inline (e.g., check
for known nested names like 'model-scalar-field' or a pattern such as
entityTypeName.includes('-')) or by calling an existing helper, and keep
listEntities(...) behavior unchanged so callers get a validation error from the
schema instead of runtime rejection.
Comment on lines
+98
to
+109
| // Load the current definition to diff against | ||
| const { container } = await loadEntityServiceContext( | ||
| input.project, | ||
| context, | ||
| ); | ||
| const currentEntityContext = container.toEntityServiceContext(); | ||
|
|
||
| const diff = diffSerializedDefinitions( | ||
| container.schema, | ||
| currentEntityContext.serializedDefinition, | ||
| session.draftDefinition, | ||
| ); |
There was a problem hiding this comment.
Diff against the draft’s original base, not the live definition.
This compares session.draftDefinition to whatever is on disk when show-draft runs. If the project definition changed after the draft was created, the output will mix unrelated on-disk edits into the draft diff instead of showing just the staged changes. Persist the original serialized definition with the session, or detect a hash mismatch and surface an out-of-date draft state instead.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/project-builder-server/src/actions/definition/show-draft.action.ts`
around lines 98 - 109, The current diff uses the live on-disk definition (via
loadEntityServiceContext -> container.toEntityServiceContext()) when calling
diffSerializedDefinitions against session.draftDefinition, which mixes unrelated
edits; change show-draft to compare session.draftDefinition against the draft’s
recorded base instead: persist the original serialized definition (e.g.,
session.baseSerializedDefinition or session.baseHash) when creating the draft
and use that value in diffSerializedDefinitions instead of
currentEntityContext.serializedDefinition, and if that persisted base is missing
or its hash mismatches the current disk definition, surface an “out-of-date
draft” state/error rather than silently diffing against the live file.
Comment on lines
+44
to
+49
| const newDefinition = updateEntity( | ||
| { | ||
| entityTypeName: input.entityTypeName, | ||
| entityId: input.entityId, | ||
| entityData: input.entityData, | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- updateEntity implementation ---'
sed -n '1,260p' packages/project-builder-lib/src/tools/entity-service/entity-write.ts
echo
echo '--- updateEntity call sites ---'
rg -n -C2 '\bupdateEntity\s*\(' \
packages/project-builder-server/src/actions \
packages/project-builder-lib/src/tools/entity-service
echo
echo '--- tests covering replacement payloads ---'
rg -n -C2 'entityData:' \
packages/project-builder-lib/src/tools/entity-service/entity-write.unit.test.tsRepository: halfdomelabs/baseplate
Length of output: 8848
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- assignEntityIds implementation ---'
rg -n -A 30 'export function assignEntityIds' packages/project-builder-lib/src/tools/
echo
echo '--- Check if there is existing-entity ID preservation logic ---'
rg -n 'preserv.*id|original.*id|existing.*id' \
packages/project-builder-lib/src/tools/assign-entity-ids.ts \
packages/project-builder-lib/src/tools/entity-service/entity-write.tsRepository: halfdomelabs/baseplate
Length of output: 2789
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- stage-update-entity input schema ---'
rg -n -B5 -A15 'stageUpdateEntity' packages/project-builder-server/src/actions/definition/stage-update-entity.action.ts | head -50
echo
echo '--- Input validation schema ---'
fd 'stage.*schema' packages/project-builder-server/srcRepository: halfdomelabs/baseplate
Length of output: 1805
Confirm: entity identity can be corrupted if entityData.id is omitted.
The input schema accepts entityData as any object (no id field required). When updateEntity calls assignEntityIds, if the payload omits id, a new ID is generated (lines 44–48 of assign-entity-ids.ts). The entity is then replaced with a different ID, breaking its identity—while the original lookup validation remains satisfied.
The fix: either require id in the input schema, or validate in updateEntity that entityData.id (if present) matches the target entityId, and inject the original ID if omitted.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@packages/project-builder-server/src/actions/definition/stage-update-entity.action.ts`
around lines 44 - 49, The update flow can accidentally change an entity's
identity when input.entityData omits id because assignEntityIds will generate a
new id; in the updateEntity path (called from stage-update-entity.action.ts)
either enforce id presence in the input schema or, simpler, validate and correct
inside updateEntity: if input.entityData.id exists ensure it equals
input.entityId (throw on mismatch), and if it is absent, set input.entityData.id
= input.entityId before calling assignEntityIds/update logic so the original
entity id is preserved; reference updateEntity and assignEntityIds when applying
this change.
kingston
deleted the
kingston/eng-1049-feat-mcp-readwrite-actions-for-project-definition-with
branch
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.
Summary by CodeRabbit
New Features
Documentation
Tests