Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
This PR will trigger a minor release when merged. |
There was a problem hiding this comment.
Hey @aniham,
Strengths
- Clean, consistent schema extension: the
intentfield is added to bothV2PromptandV2PromptInputwith proper nullable typing, an explicit enum, a clear description, and an example value (docs/openapi/schemas.yaml). - Well-scoped change with good change discipline: the diff touches exactly the layers it needs (OpenAPI schema, controller filter dimensions, storage read/write/update, tests, integration seed infrastructure) and nothing else.
- Filter implementation follows established patterns: the
intentfilter uses the samehasText()guard and.eq()call as the adjacentoriginfilter (src/support/prompts-storage.js:386-388). - Parameterized query via PostgREST client (
.eq('intent', intent)) - no raw string interpolation, no injection surface. - Hardcoded frozen constant for filter-dimensions (
CONFIG_PROMPT_INTENTS) is the correct pattern for a schema-defined enum, consistent withCONFIG_FILTER_ORIGINSabove it. - Integration seed ordering fix (
test/it/postgres/seed.js) correctly movesbrandsto Level 2 (aftersites) to account for the newsite_idforeign key constraint.
Issues
Important (Should Fix)
1. Missing integration test for intent filter and prompt CRUD
test/it/postgres/directory- The CLAUDE.md rule states: "New or modified endpoints must include integration tests in
test/it/." This PR modifies the prompts listing behavior (newintentfilter), upsert (persistsintent), and update (patchesintent). The docker-compose image bump and seed fixes suggest IT infrastructure was updated to support the column, but no integration test exercises the new functionality end-to-end. Unit tests mock the PostgREST client, so they cannot validate that the DB column exists, that the filter works against real data, or that the data-service image (v5.28.1) includes theintentcolumn in its schema. - Fix: Add at least one integration test that seeds a prompt with an
intentvalue, retrieves it (verifying the field appears in the response), and filters byintent.
2. Missing intent query parameter declaration in OpenAPI endpoint parameters
docs/openapi/(endpoint path definition, not shown in diff)- The diff adds
intentto theV2PromptandV2PromptInputmodel schemas and the code acceptsintentas a filter inlistPrompts. However, the diff does not show a corresponding query parameter being added to the endpoint'sparameterssection in the OpenAPI paths file. If the parameter is not declared there, API consumers relying on the spec for discoverability will not find it. - CLAUDE.md states: "Specification Sync: Keep OpenAPI specs and implementation in sync."
- Fix: Add
intentas a query parameter (string, nullable, with the same enum) to the relevantGETendpoint definition. If it is already declared elsewhere not shown in this diff, disregard.
Minor (Nice to Have)
3. Hardcoded intent enum duplicated across schema and code
docs/openapi/schemas.yaml(lines 6199, 6284) andsrc/controllers/llmo/llmo-brand-presence.js:647-651- The list
[informational, instructional, comparative, transactional, planning, delegation]now lives in the OpenAPI schema (twice, for Prompt and PromptInput) and inCONFIG_PROMPT_INTENTS. If a new intent is added later, all locations must be updated in lockstep. For a small, stable enum this is tolerable, but consider extracting to a shared constant if the list is expected to grow.
Recommendations
- Consider server-side enum validation on write paths:
upsertPromptsandupdatePromptByIdusep.intent || null/updates.intent || nullwhich coerces falsy values tonull. This is fine given the OpenAPI enum at the boundary, but if internal callers can reach these functions without passing through the API validation layer, invalid intent values could be persisted silently. AVALID_INTENTS.has(intent)guard would provide defense-in-depth.
Assessment
Ready to merge? With fixes
The core logic is well-structured, follows existing codebase patterns, and the feature is properly additive (no breaking changes). The two Important findings are about completeness - the integration test gap (required by project conventions) and the query parameter declaration (required for API discoverability). Both are straightforward additions that would bring this to merge-ready state.
Skill: pr-review | Model: us.anthropic.claude-opus-4-6-v1[1m] | Duration: 1m 27s | Cost: $3.44 | Commit: b3692fc1f402cf18a1592404cd6913f6c2a6da22
If this code review was useful, please react with 👍. Otherwise, react with 👎.
|
The description "Null for human-created prompts" doesn't align with the existing DRS architecture. All prompts should have |
https://jira.corp.adobe.com/browse/LLMO-5161