Skip to content

feat: return prompt intent in llmo bp | LLMO-5161#2535

Open
aniham wants to merge 5 commits into
mainfrom
LLMO-5161
Open

feat: return prompt intent in llmo bp | LLMO-5161#2535
aniham wants to merge 5 commits into
mainfrom
LLMO-5161

Conversation

@aniham

@aniham aniham commented Jun 1, 2026

Copy link
Copy Markdown
Contributor

@codecov

codecov Bot commented Jun 1, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@github-actions

github-actions Bot commented Jun 1, 2026

Copy link
Copy Markdown

This PR will trigger a minor release when merged.

@aniham aniham temporarily deployed to dev-branches June 2, 2026 00:54 — with GitHub Actions Inactive
@aniham aniham requested a review from nsdere June 2, 2026 01:34

@MysticatBot MysticatBot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @aniham,

Strengths

  • Clean, consistent schema extension: the intent field is added to both V2Prompt and V2PromptInput with 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 intent filter uses the same hasText() guard and .eq() call as the adjacent origin filter (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 with CONFIG_FILTER_ORIGINS above it.
  • Integration seed ordering fix (test/it/postgres/seed.js) correctly moves brands to Level 2 (after sites) to account for the new site_id foreign 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 (new intent filter), upsert (persists intent), and update (patches intent). 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 the intent column in its schema.
  • Fix: Add at least one integration test that seeds a prompt with an intent value, retrieves it (verifying the field appears in the response), and filters by intent.

2. Missing intent query parameter declaration in OpenAPI endpoint parameters

  • docs/openapi/ (endpoint path definition, not shown in diff)
  • The diff adds intent to the V2Prompt and V2PromptInput model schemas and the code accepts intent as a filter in listPrompts. However, the diff does not show a corresponding query parameter being added to the endpoint's parameters section 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 intent as a query parameter (string, nullable, with the same enum) to the relevant GET endpoint 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) and src/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 in CONFIG_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: upsertPrompts and updatePromptById use p.intent || null / updates.intent || null which coerces falsy values to null. 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. A VALID_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 👎.

@MysticatBot MysticatBot added the ai-reviewed Reviewed by AI label Jun 2, 2026
@nsdere

nsdere commented Jun 2, 2026

Copy link
Copy Markdown
Contributor

The description "Null for human-created prompts" doesn't align with the existing DRS architecture. _hydrate_user_intent in hydration_runner.py applies to all prompts regardless of origin — the docstring explicitly states: "Applies to ALL prompts (both AI and human origin)." Human prompts go through UserIntentClassifier the same way AI-generated ones do, just at BP analysis time rather than generation time.

All prompts should have intent populated. Suggest updating the schema description to reflect this.

@aniham aniham temporarily deployed to dev-branches June 2, 2026 18:06 — with GitHub Actions Inactive
@aniham aniham temporarily deployed to dev-branches June 3, 2026 12:27 — with GitHub Actions Inactive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ai-reviewed Reviewed by AI

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants