feat(api): AI tag suggestion endpoint (#223)#259
Conversation
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Code Review: PR #259 — feat(api): AI tag suggestion endpoint
Scope: PR #259 in thomasluizon/orbit-api
Recommendation: NEEDS WORK
Summary
PR #259 adds POST /api/tags/suggest — an AI-backed tag-suggestion endpoint that gates on the shared pay gate, reuses existing tags by case-insensitive name matching, caps at MaxTagsPerHabit, and meters one AI message on success. The implementation is clean and mirrors the established AiGoalReviewService pattern faithfully. One High finding blocks merge: the Language field accepted by the new endpoint is not validated against AppConstants.SupportedLanguages or AppConstants.MaxLanguageLength, while every other Language-accepting endpoint in the codebase enforces both constraints via SetLanguageCommandValidator's pattern.
Findings
Critical
None.
High
[HIGH] Language field unvalidated in SuggestTagsQueryValidator
- dimension: Backend hard rules (#13) — Validation; Security (#12) — Validation
- location:
src/Orbit.Application/Tags/Validators/SuggestTagsQueryValidator.cs - issue:
SuggestTagsQueryValidatorhas rules forUserId,Title, andDescriptionbut no rule forLanguage.AppConstants.MaxLanguageLength = 10andAppConstants.SupportedLanguages = ["en", "pt-BR"]exist specifically for this purpose, andSetLanguageCommandValidator(src/Orbit.Application/Profile/Validators/SetLanguageCommandValidator.cs:14-18) already uses them as the canonical pattern. An unsupported language value silently falls back to"English"insideLocaleHelper.GetAiLanguageName— the prompt still executes, the AI message is still metered, and the caller gets English suggestions regardless of intent. - risk: An arbitrarily long
Languagestring reaches the service layer unchecked. The larger risk is a broken validation contract: the codebase invariant is that everyLanguage-bearing field is validated against the allowlist, and this endpoint breaks that invariant. - fix: Add to
SuggestTagsQueryValidator:
RuleFor(x => x.Language)
.MaximumLength(AppConstants.MaxLanguageLength)
.Must(lang => AppConstants.SupportedLanguages.Contains(lang, StringComparer.OrdinalIgnoreCase))
.WithMessage($"Language must be one of: {string.Join(", ", AppConstants.SupportedLanguages)}");- reference: orbit-api CLAUDE.md hard rule "Validation: validators in
Orbit.Application/<Feature>/Validators/"; CLAUDE.md "The backend is the source of truth — never trust the frontend."
Medium
None.
Low / Info
None.
Subagents
| Agent | Verdict |
|---|---|
| security-reviewer | FAIL — 1 High (Language unvalidated); other findings downgraded (see notes) |
| contract-aligner | NOT VERIFIABLE IN CI — orbit-ui-mobile not checked out; API shape verified correct |
Security reviewer finding adjustments:
-
"Missing DistributedRateLimit" (flagged Medium): downgraded to N/A.
GetRetrospective(HabitsController) andGetGoalReview(GoalsController) are also AI-backed, also rely on the pay gate, and also have no[DistributedRateLimit]. This is the established codebase pattern for pay-gated AI endpoints. -
"TOCTOU between gate check and meter increment" (flagged Medium): downgraded to N/A. The chat handler (
ProcessUserChatCommand.Persistence.cs) meters AI messages in a background fire-and-forget task after the AI response completes — an even more permissive ordering. TheSuggestTagshandler meters synchronously after the AI call, which is strictly tighter. Pre-existing pattern, not introduced here. -
"Unbounded AI tag name length in response" (flagged Low): note only. If a caller passes a returned name to
POST /api/tags,Tag.Create()enforces the 50-char domain guard. No new storage path introduced here.
Contract aligner notes: The API-side JSON shape is correct (tags[].name, tags[].color, tags[].isExisting, tags[].id). The id field is Guid? and serialises to a UUID string or null (never omitted) — the consumer Zod schema must use .nullable() not .optional(). Verification of the orbit-ui-mobile#223 consumer side requires checking out that repo.
Validation
| Check | Result |
|---|---|
| Build (dotnet) | N/A (--skip-build) |
| Tests (dotnet) | N/A (--skip-build) |
What's good
- Clean layering: interface in Domain, service in Infrastructure, handler in Application — mirrors
AiGoalReviewService/IGoalReviewServiceexactly. - Pay gate is the first call in the handler; AI is never invoked when the user is over quota.
MapSuggestionsis a pure static method; case-insensitive deduplication withHashSet<string>(OrdinalIgnoreCase)andexistingByNamedictionary with the same comparer is correct.CapitalizematchesTag.Create()'s own normalization logic exactly — returned names are consistent with stored names.- Prompt uses structured output shape and instructs the model to prefer reuse over near-duplicates.
LogGeneratingTagSuggestionslogs onlylanguage— no PII (title/description) reaches the log.AgentCatalogService.Capabilities.cscorrectly addsSuggestTagsto theTagsReadcapability'scontrollerActions.- Test suite covers all five meaningful scenarios: pay-gate fail (no AI call, no metering), AI fail (no metering), new suggestion + metering, case-insensitive existing reuse, dedupe + cap.
Recommendation
One change before merge: add a Language validation rule to SuggestTagsQueryValidator following SetLanguageCommandValidator's existing pattern. Three-line addition, no structural changes needed.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Code Review: PR #259 — feat(api): AI tag suggestion endpoint (#223)
Scope: PR #259 in thomasluizon/orbit-api
Recommendation: NEEDS WORK
Summary
PR #259 adds a clean, well-tested POST /api/tags/suggest endpoint that feeds a habit title and optional description into gpt-4.1-mini (SubTask tier) to propose 1–5 tags, preferring reuse of existing tags over new duplicates. The handler, validator, DI wiring, and tests are all present and follow established patterns. Two issues block merge: the AI endpoint lacks a rate-limit attribute (every other AI-invoking endpoint in the codebase has one), and the new action is mis-catalogued as isMutation: false in the agent capability registry even though it commits a metered AI-message increment to the database. A localization defect in the Capitalize helper is also flagged.
Findings
Critical
None.
High
[HIGH] Missing [DistributedRateLimit] on SuggestTags
· dimension: Security (#12 — API abuse / unrestricted resource consumption)
· location: src/Orbit.Api/Controllers/TagsController.cs:104
· issue: POST /api/tags/suggest makes a paid OpenAI API call on every invocation. Every other AI-invoking endpoint in the codebase carries a [DistributedRateLimit] attribute — ChatController at class level ("chat"), AiController.ResolveConflict at action level ("ai-resolve"), AuthController on every auth action. SuggestTags has no rate-limit guard, so an authenticated user (or a compromised account) can fire an unbounded burst of concurrent requests. The CanSendAiMessage quota check is a logical gate, not a timing gate; concurrent requests all pass it before the first meter commit lands.
· risk: Uncontrolled API spend; an attacker or runaway client can exhaust the OpenAI budget in seconds.
· fix: Add [DistributedRateLimit("ai")] (or reuse "chat") immediately above the [HttpPost("suggest")] attribute, mirroring the AiController pattern.
· reference: orbit-api Security hard rule — "Authorization / rate limiting"; OWASP API4:2023 Unrestricted Resource Consumption.
[HIGH] SuggestTags catalogued as isMutation: false in agent capability registry
· dimension: Correctness (#1)
· location: src/Orbit.Infrastructure/Services/AgentCatalogService.Capabilities.cs:383
· issue: TagsRead capability sets isMutation: false but now lists "TagsController.SuggestTags" in its controllerActions. The handler calls user.IncrementAiMessageCount() and commits via unitOfWork.SaveChangesAsync, making it a state-mutating operation. Mis-classifying it as a read means the agent-layer policy engine will not apply write-path guards (confirmation requirements, audit logging) when an MCP or chat client invokes this action.
· risk: An agent operating under a read-only scope can silently consume AI message quota, bypassing write-path policy that would otherwise require confirmation or logging.
· fix: Move "TagsController.SuggestTags" into a new dedicated capability (e.g. TagsSuggestRead) with isMutation: true and a planRequirement consistent with the pay-gate, mirroring the DailySummaryRead capability shape. Keeping it under the free list-read capability obscures the cost boundary.
· reference: Correctness (#1) — data-flow integrity; orbit-api CLAUDE.md "No workarounds / root-cause every bug."
Medium
[MEDIUM] Capitalize uses culture-sensitive char.ToUpper / .ToLower() without InvariantCulture
· dimension: Correctness (#1) / Backend hard rules (#13)
· location: src/Orbit.Application/Tags/Queries/SuggestTagsQuery.cs:97
· issue: char.ToUpper(value[0]) + value[1..].ToLower() uses the thread's ambient CultureInfo. For Turkish (tr-TR), char.ToUpper('i') yields İ (dotted capital I) and .ToLower('I') yields ı (dotless lowercase I), producing incorrect tag capitalization. The existing ProcessUserChatCommand.ToolResults.cs:96 already uses the correct char.ToUpper(p[0], CultureInfo.InvariantCulture) form; the pre-existing helpers in CreateHabitTool.cs and AssignTagsTool.cs share the same defect.
· risk: Tag names silently mangled on a Turkish-locale server thread; case-insensitive deduplication may also behave unexpectedly.
· fix:
private static string Capitalize(string value) =>
string.IsNullOrEmpty(value)
? value
: char.ToUpper(value[0], CultureInfo.InvariantCulture) + value[1..].ToLower(CultureInfo.InvariantCulture);· reference: Backend hard rules (#13); established codebase pattern in ProcessUserChatCommand.ToolResults.cs:96.
Low / Info
None.
Subagents
| Agent | Verdict |
|---|---|
| security-reviewer | FAIL — 1 High (missing rate limit), 2 Medium (prompt injection defense-in-depth gap, capability mis-classification) |
| contract-aligner | NOT VERIFIABLE IN CI — orbit-ui-mobile not checked out; 0 API-side drifts detected |
Validation
| Check | Result |
|---|---|
| Build (dotnet) | N/A — skipped per CI instruction |
| Tests (dotnet) | N/A — skipped per CI instruction |
What's good
- Pay-gate check precedes any AI call; metering is skipped on failure — no quota consumed unless the suggestion succeeds.
MapSuggestionsis a clean static method with no hidden I/O; deduplication, cap, and case-insensitive matching are all directly testable and tested.AiTagSuggestionService.BuildPromptisinternal static, enabling direct unit-test coverage — all placeholder branches (no description, no existing tags, non-English language) are covered.Languageis validated againstAppConstants.SupportedLanguagesand defaults to"en"at the controller boundary — no unsanitized language value reaches the prompt.[LoggerMessage]source-generated log methods throughout; no string interpolation in log calls.- Test coverage is comprehensive: pay-gate failure, AI failure, new-tag mapping, existing-tag reuse, case-insensitive deduplication, and
MaxTagsPerHabitcap are all exercised. - Error propagation follows the established
PropagateError<T>()/ToPayGateAwareResult()pattern. - No EF/schema changes — purely additive.
Recommendation
Two changes are required before merge:
- Add
[DistributedRateLimit("ai")](or"chat") toTagsController.SuggestTags(line 104) — one-line fix matching every other AI-cost endpoint. - Correct the agent capability classification in
AgentCatalogService.Capabilities.cs: move"TagsController.SuggestTags"into a dedicatedTagsSuggestReadcapability withisMutation: trueand aplanRequirement, mirroringDailySummaryRead.
The Capitalize / InvariantCulture fix (Medium) is a one-liner and affects two other pre-existing helpers — worth batching into this PR since it's touching the same helper pattern.
🤖 Generated with Claude Code
| return result.ToPayGateAwareResult(() => NoContent()); | ||
| } | ||
|
|
||
| [HttpPost("suggest")] |
There was a problem hiding this comment.
[HIGH] Missing [DistributedRateLimit] on SuggestTags
Every other AI-invoking endpoint in the codebase carries a [DistributedRateLimit] attribute — ChatController at class level ("chat"), AiController.ResolveConflict at action level ("ai-resolve"). This endpoint makes a paid OpenAI call on every invocation; without the attribute, concurrent authenticated requests all pass the logical CanSendAiMessage quota gate before the first meter commit lands, enabling unbounded API spend.
| [HttpPost("suggest")] | |
| [DistributedRateLimit("ai")] | |
| [HttpPost("suggest")] |
| chatTools: ["list_tags"], | ||
| mcpTools: ["list_tags"], | ||
| controllerActions: ["TagsController.GetTags"]), | ||
| controllerActions: ["TagsController.GetTags", "TagsController.SuggestTags"]), |
There was a problem hiding this comment.
[HIGH] SuggestTags mis-catalogued as isMutation: false
The SuggestTagsQueryHandler calls user.IncrementAiMessageCount() and commits via unitOfWork.SaveChangesAsync, making it a state-mutating operation. Listing it under TagsRead (a read-only, free capability) means an agent operating under a read-only scope can silently consume AI message quota without write-path guards (confirmation requirements, audit logging).
Fix: move "TagsController.SuggestTags" into a new dedicated capability (e.g. TagsSuggestRead) with isMutation: true and a planRequirement, mirroring the DailySummaryRead capability shape.
#223) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
PR #259 Review
Decision: APPROVE — 0 Critical, 0 High findings.
Medium
Capitalize is a third private copy of a utility already duplicated in two other files
Location: src/Orbit.Application/Tags/Queries/SuggestTagsQuery.cs:96-97
The same helper exists verbatim in Chat/Tools/Implementations/CreateHabitTool.cs:328-329 and Chat/Tools/Implementations/AssignTagsTool.cs:147-148. Three copies diverge independently — and they already have: the ProcessUserChatCommand.ToolResults.cs variant correctly passes CultureInfo.InvariantCulture to char.ToUpper; all three Capitalize copies use the culture-sensitive overload (subtly wrong for Turkish/Azerbaijani locales on tag name normalisation).
Fix: Extract static string Capitalize(string value) into Orbit.Application/Common/StringExtensions.cs using char.ToUpper(value[0], CultureInfo.InvariantCulture) and replace all three call sites. Can follow in a tidy-up PR.
Info / Low (no action required before merge)
Missing [ProducesResponseType(StatusCodes.Status429TooManyRequests)] on SuggestTags — pre-existing gap across all rate-limited endpoints, not introduced here. Address in a dedicated chore PR.
Cross-repo contract not verifiable in CI — endpoint is purely additive (no existing fields removed or renamed). The orbit-ui-mobile session must add SuggestedTagSchema, SuggestTagsResponseSchema, and a POST /api/tags/suggest constant in endpoints.ts before the feature is callable from the client.
What's good
- Pay gate is correctly ordered:
CanSendAiMessagefires before any DB fetch, AI call, or metering; gate failures are cheap. - Rate limit wired correctly: bucket
"tag-suggest"is registered inDistributedRateLimitService; a typo would throw at runtime rather than silently bypassing. - Language allowlist enforced: arbitrary strings cannot enter
BuildPrompt. - Authorization is airtight: class-level
[Authorize];UserIdsourced fromHttpContext.GetUserId()(JWT), not the request body. - Error handling is clean: AI exceptions map to
AiTagSuggestionUnavailable;OperationCanceledExceptionpropagates naturally; no stack traces reach the caller. MapSuggestionsis correct: case-insensitive dedup, cap atMaxTagsPerHabit, existing tagId/Colorpreferred over AI-generated values.- Test coverage is solid: pay-gate-fail, AI-fail-no-metering, new-tag mapping, existing-name reuse, case-insensitive dedup + cap, all validator branches, all prompt content variants.
- Agent catalog updated correctly:
TagsController.SuggestTagsinTagsWrite,isMutation: true,AgentRiskClass.Low.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
AI Tag Suggestion Endpoint review: APPROVE. No Critical or High findings. Two Medium and two Info items.
MEDIUM - Metering bypass (SuggestTagsQuery.cs:46-51): MeterAiMessageAsync is only called on the success branch. If the OpenAI call completes but returns null or empty Tags list, AiEmptyResponse is returned and the counter is never incremented. A user can loop this path uncharged. Fix: call MeterAiMessageAsync right after the pay-gate check, before dispatching to the service.
MEDIUM - Prompt injection (AiTagSuggestionService.cs:69-83): BuildPrompt embeds raw title and descriptionLine verbatim in the prompt. Instruction-shaped input can interfere with the RULES block. Fix: wrap user content in XML delimiters and move RULES above the user-data section. (OWASP LLM Top 10 2025 LLM01)
INFO - SuggestTags cataloged under TagsWrite but never mutates (AgentCatalogService.Capabilities.cs:404): isMutation:true overstates the required scope. Consider TagsRead or a dedicated TagsSuggest capability.
INFO - Consumer-side Zod schemas and endpoint constant not verifiable in CI. companion orbit-ui-mobile PR must add endpoints.ts constant, SuggestedTagSchema, SuggestTagsResponseSchema, Server Action, and apiClient call.
Pattern fidelity, authorization, rate limiting, validation, deduplication logic, and test coverage are all correct. Backward compatible - no existing contracts broken.
|
There was a problem hiding this comment.
Code Review: PR #259 — feat(api): AI tag suggestion endpoint (#223)
Scope: PR #259 in thomasluizon/orbit-api
Recommendation: APPROVE
Summary
This PR adds a well-structured POST /api/tags/suggest endpoint that calls the cheap SubTask AI model to propose 1–5 tags for a habit, reusing existing tags case-insensitively and metering one AI message on success. The implementation mirrors the existing AI service pattern faithfully (validators, pay-gate, error propagation, rate-limit, structured logging). No EF schema change. All new code is exercised by unit tests covering pay-gate failure, AI failure, new-tag mapping, existing-name reuse, dedupe, and the per-habit cap.
Findings
Critical
None.
High
None.
Medium
[MEDIUM] SuggestTags catalogued as a write (mutation) action despite being a read-only query
· dimension: 13. Backend hard rules / SOLID / agent catalog correctness
· location: src/Orbit.Infrastructure/Services/AgentCatalogService.Capabilities.cs:404
· issue: TagsController.SuggestTags is listed in the TagsWrite capability (isMutation: true). The handler is a SuggestTagsQuery — it reads user tags, calls AI, and meters a counter, but it does not create, mutate, or delete any domain aggregate. The AI-message meter (User.IncrementAiMessageCount) is a side-effect of read queries in other features too (e.g., summary/retrospective); those are not catalogued as writes.
· risk: An agent granted only TagsRead cannot call SuggestTags. Conversely, granting TagsWrite to call SuggestTags implicitly unlocks CreateTag/UpdateTag/AssignTags as collateral write scope — broader than intended. Phase-one read-only mode would also unnecessarily block a non-destructive suggestion call.
· fix: Move "TagsController.SuggestTags" from the TagsWrite controllerActions list to the TagsRead controllerActions list (consistent with query intent and other AI-suggestion endpoints). Alternatively, add a dedicated TagsSuggest capability if AI-metering warrants its own scope boundary.
· reference: orbit-api CLAUDE.md clean-arch; AgentCatalogService pattern
Low / Info
None posted (signal gate).
Subagents
| Agent | Verdict |
|---|---|
| security-reviewer | PASS — [Authorize] inherited at class level; rate-limit wired; GetUserId() from JWT claim; no PII in logs; structured-log calls source-generated. One medium finding (unbounded existingTagNames token cost in BuildPrompt); one low prompt-injection structural note — both below the signal gate. |
| contract-aligner | NOT VERIFIABLE IN CI — orbit-ui-mobile not checked out; API side internally consistent (route, request DTO, response DTOs all coherent). Consumer-side endpoints.ts constant + Zod schemas must be present in orbit-ui-mobile before the endpoint is wired up in the apps. |
Validation
| Check | Result |
|---|---|
| Build (dotnet) | N/A (--skip-build) |
| Tests (dotnet) | N/A (--skip-build) |
What's good
- Clean mirroring of the established AI service pattern (
AiGoalReviewService,AiRescheduleSuggestionService): interface in Domain, implementation in Infrastructure, handler in Application, all wired via DI. Capitalize+ case-insensitiveHashSetdedup +AppConstants.MaxTagsPerHabitcap work together correctly — the AI cannot return more tags than the per-habit limit regardless of what it generates.- Rate limit entry added to
DistributedRateLimitServiceat 15 req/min, matchinghabit-suggest— appropriate for the AI cost. - Metering fires only on success (after the
suggestionResult.IsFailureguard), so failed AI calls do not burn the user's monthly quota. TagSuggestionResultis aprivate sealed record— no leakage of the internal deserialization type.- All log calls use source-generated
[LoggerMessage]— no string interpolation in production log paths, no PII. - Test suite is thorough: pay-gate fast-path (no AI call, no metering), AI failure path (no metering), new-tag capitalization, case-insensitive existing-name reuse with real Id/Color passthrough, dedupe + cap edge case all covered.
AiTagSuggestionServiceTeststestsBuildPromptas aninternal staticmethod directly — a clean approach that avoids mocking the AI client for prompt-correctness assertions.
Recommendation
One medium finding to address: "TagsController.SuggestTags" belongs in TagsRead.controllerActions, not TagsWrite. This is a one-line fix in AgentCatalogService.Capabilities.cs. The endpoint itself is correct and safe; this can ship as a follow-up chore PR.
Before this endpoint is called in the apps, confirm orbit-ui-mobile#223 adds the endpoints.ts constant and Zod schemas for SuggestTagsResponse / SuggestedTag.
🤖 Generated with Claude Code


What
Adds
POST /api/tags/suggest— an AI tag-suggestion endpoint for the habit create/edit form. Given a habit title + optional description, it fetches the user's existing tags and asks the cheapSubTaskmodel (via the existingAiCompletionClient.CompleteJsonAsync) to propose 1–5 short tags that prefer reusing existing tags over creating duplicates.ITagSuggestionService(Domain) +AiTagSuggestionService(Infrastructure): prompt builder + structured-JSON completion onAiModelTier.SubTask, mirroringAiGoalReviewServiceerror handling.SuggestTagsQuery+ handler: gates onIPayGateService.CanSendAiMessage, maps returned names case-insensitively against existing tags (reuse → real id + color; new → default#7c3aed), dedupes + caps atAppConstants.MaxTagsPerHabit, then meters one AI message viaUser.IncrementAiMessageCount().SuggestTagsQueryValidator,TagsController.SuggestTags(mapped viaresult.ToPayGateAwareResult), DI registration,ErrorMessages.AiTagSuggestionUnavailable, and aTagsReadagent-catalog mapping for the new action.Why
Closes the API half of AI auto-tagging (orbit-ui-mobile#223). Metering draws from the shared Astra chat allowance (not Pro-only, not a separate budget). No EF/schema change. The endpoint is purely additive — old mobile clients simply never call it — so there is no backward-compat break.
Tests
SuggestTagsQueryHandlerTests: pay-gate-fail (no AI call, no metering), new-suggestion mapping + metering, existing-name reuse (real id + color), case-insensitive dedupe + cap at the per-habit limit, AI-failure propagation.AiTagSuggestionServiceTests: prompt embeds title/description/existing tags + target language; placeholders for empty inputs.Refs thomasluizon/orbit-ui-mobile#223
🤖 Generated with Claude Code