♻️ Refactor: Prompt service adapter - #16469
AtefBellaaj wants to merge 1 commit into
Conversation
|
@codex review |
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. |
![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: 67a373aa3c
ℹ️ 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".
| if (input.group.name.trim().length === 0) { | ||
| return invalidInput('Prompt and group name are required'); | ||
| } |
There was a problem hiding this comment.
When a create request contains an empty prompt or an invalid prompt type, this checks only the group name and still calls source.createGroup; unlike addRevision, the identical prompt shape never passes through validateRevisionInput. A native source will reject this at the database layer and turn a client input error into a 500, while a less strict source could persist an unusable revision, so validate the complete creation payload before writing.
AGENTS.md reference: AGENTS.md:L103-L115
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 3a28bcf. Group creation now runs the shared prompt validator before calling the source. The regression test covers whitespace-only text and an invalid prompt type. Focused prompt tests and TypeScript pass.
| readonly updates: TUpdatePromptGroupSchema; | ||
| readonly filters?: ResolvePromptInput['filters']; | ||
| }): Promise<PromptServiceResult<PromptGroupRecord>> { | ||
| const updates = validatePromptGroupUpdate(input.updates); |
There was a problem hiding this comment.
When an update contains an unknown field, an overlong value, or an invalid command, validatePromptGroupUpdate throws a ZodError even though updateGroup promises a PromptServiceResult. Once the existing route delegates to this service, these expected client errors bypass the invalid_input contract and are liable to become generic 500 responses; use the safe validator and return the typed failure instead.
AGENTS.md reference: AGENTS.md:L103-L115
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 3a28bcf. Group updates now use the existing safe validator and return invalid_input without writing. The regression test covers a strict-schema rejection. Focused prompt tests and TypeScript pass.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3a28bcf23b
ℹ️ 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: 7c80cc23c3
ℹ️ 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: 1f80eccf5c
ℹ️ 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".
| listRevisions: dependencies.getPrompts, | ||
| createGroup: dependencies.createPromptGroup, | ||
| addRevision: dependencies.savePrompt, | ||
| promoteRevision: dependencies.makePromptProduction, |
There was a problem hiding this comment.
When this adapter is wired to the current makePromptProduction method, a Mongo read or update failure is caught by packages/data-schemas/src/methods/prompt.ts and resolved as { message: 'Error making prompt production' }, which has the same type as its success response. Directly delegating here lets service.promoteRevision wrap that failure in { ok: true }, so a caller can return success even though the production revision was unchanged; make the native dependency throw or expose a discriminated failure before passing it through the service.
AGENTS.md reference: AGENTS.md:L116-L121
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 98aa11c. The native adapter now accepts only the established production-success response. It throws when the data method returns its error message, so the service cannot wrap a failed mutation as success. A regression test covers the error response.
| function validateRevisionInput<T>( | ||
| input: Pick<AddPromptRevisionInput, 'prompt'>, | ||
| ): PromptServiceResult<T> | null { | ||
| if (typeof input.prompt.prompt !== 'string' || input.prompt.prompt.trim().length === 0) { |
There was a problem hiding this comment.
For an untrusted create or add-revision payload where prompt is missing or null, this dereference throws a TypeError rather than returning the service's invalid_input result; the existing HTTP route treats this case as a client error. Fresh evidence after 1f80ecc is that validation now safely handles the group name but still reads input.prompt.prompt without first validating input.prompt, so validate the nested object before its fields.
AGENTS.md reference: AGENTS.md:L103-L115
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 98aa11c. The shared revision validator now checks that prompt is a non-null object before reading text or type. Regression coverage includes null creation input and missing add-revision input. The focused 35 tests and TypeScript pass.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 98aa11c428
ℹ️ 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".
| const result = await dependencies.makePromptProduction(promptId); | ||
| if (result.message !== 'Prompt production made successfully') { | ||
| throw new Error(result.message); |
There was a problem hiding this comment.
When an existing prompt references a deleted or tenant-inaccessible group, PromptGroup.findByIdAndUpdate in packages/data-schemas/src/methods/prompt.ts resolves to null without throwing, but the method ignores that result and still returns the exact success message accepted here. The service therefore reports { ok: true } although no production revision changed. Fresh evidence after 98aa11c is that the update's null result remains unchecked; require the native method to verify that a group was updated before returning the success response.
AGENTS.md reference: AGENTS.md:L116-L121
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
@codex This adapter is intentionally not wired directly to the current PromptMethods implementation. NativePromptDependencies is a port that requires promotion to reject unless the group update succeeded. The legacy method cannot satisfy that contract because it discards the update result, so AI-2154 must fork the concrete native operation during runtime wiring. Changing the shared legacy method in this isolated spike would affect the old path and violate the repository reuse-or-fork rule. Please check.
| loadedGroup?._id === groupId ? loadedGroup : await dependencies.getPromptGroup(groupId); | ||
| if (group == null || group.productionId == null) { | ||
| return null; |
There was a problem hiding this comment.
When production resolution is backed by the current getPromptGroup data method and its Mongo aggregation fails, that method catches the exception and returns null (packages/data-schemas/src/methods/prompt.ts:724-726). This branch interprets the same value as an absent group, so resolvePrompt reports unavailable_selection instead of propagating the database outage; change the native lookup boundary so failures throw and reserve null for a successful lookup with no record.
AGENTS.md reference: AGENTS.md:L108-L109
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
@codex NativePromptDependencies.getPromptGroup already defines null as a successful missing-record result. Promise rejection is the operational-failure path. The current legacy method swallows storage errors, so it does not satisfy this port and cannot be injected directly. AI-2154 owns the forked concrete operation when runtime wiring is added; this draft explicitly has no runtime caller. Changing the old shared method here would affect the existing path and violate the repository reuse-or-fork rule. Please check.
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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: 98aa11c428
ℹ️ 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".
| const prompt = input.prompt as { readonly prompt?: unknown; readonly type?: unknown }; | ||
| if (typeof prompt.prompt !== 'string' || prompt.prompt.trim().length === 0) { | ||
| return invalidInput('Prompt text is required and must be a non-empty string'); | ||
| } | ||
| if (prompt.type !== 'text' && prompt.type !== 'chat') { |
There was a problem hiding this comment.
When a future HTTP caller passes extra properties under prompt, such as _id or tenantId, this validator accepts them because it checks only prompt and type; createGroup and addRevision then forward the original object at lines 178 and 206 rather than a sanitized value. The native prompt schema explicitly persists tenantId (packages/data-schemas/src/schema/prompt.ts:27-30), and Mongoose also honors a supplied _id, so malformed client input can control stored metadata or trigger duplicate-key operational failures instead of returning invalid_input. Parse with a strict prompt schema and pass only its validated output to the source.
AGENTS.md reference: AGENTS.md:L103-L115
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in e77bd94. Create and add-revision now parse prompt data with one strict shared schema and pass only its validated output to the source. Extra tenantId and _id fields return invalid_input before any write. The focused schema, service, and adapter suites have 62 passing tests, and TypeScript passes.
|
Codex Review: Didn't find any major issues. Nice work! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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: a5ffa825ad
ℹ️ 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".
| input.loadedGroup?._id === input.groupId | ||
| ? input.loadedGroup | ||
| : await source.getGroup(input.groupId); |
There was a problem hiding this comment.
When the caller has already looked up the requested group and passes loadedGroup: null, this condition treats the documented absence as if nothing was loaded and performs another database read. Besides defeating the preload contract, a record created between the two reads can make the service return a different result from the lookup on which the caller based authorization or request handling. Distinguish an omitted preload (undefined) from a supplied null; the same issue occurs for loadedRevision and in the native resolver.
AGENTS.md reference: AGENTS.md:L108-L109
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 47e56ff. A supplied null preload now remains a known missing result in service group reads, revision reads, promotion, and both native resolver paths. None of these paths performs a second lookup. Regression tests cover all affected paths, all 70 focused prompt tests pass, and the API package builds. @codex please check.
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
Codex Review: Didn't find any major issues. Another round soon, please! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
AtefBellaaj
changed the title
47e56ff to
790dd98
Compare
c6b0a1b to
baedb6a
Compare
|
closed in favor of #16506 |
1 participant
Summary
Native prompt behavior is currently coupled to route handlers and database methods. This makes it difficult to add another prompt source without copying selection, protection, and mutation rules.
This draft spike proposes a shared prompt service and a native source adapter for AI-2154. It is intentionally not wired into the HTTP routes. The goal is to review the contracts and test shape before the production refactor.
How it works
The service owns validation, operation-specific content protection, catalog projection, Usage, and best-effort creator ownership grants. The native adapter owns Production and exact-revision selection plus one-to-one native reads and mutations. Both boundaries use plain string IDs and injected dependencies.
Type of change
Testing
Tested environments/configuration:
Automated tests:
npm run test:ci -- --runInBand --coverage=falseinpackages/api: 609 suites passed, 16,090 tests passed, 13 skipped by normal suite filtersnpx tsc --noEmitinpackages/apinpm run build:apinpm run static-checks:full: all affected checks passed; unused-package check skipped because globaldepcheckis not installedScreenshots / recordings
No user-facing change.
Risk / compatibility
This is a draft architecture spike. It exports the proposed service, adapter, and contracts, but no route or runtime caller uses them. It changes no HTTP response, database schema, stored data, or configuration. The shared prompt protection input now accepts the existing nullable command shape.
Checklist