Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions docs/openapi/prompts-v2-api.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -254,6 +254,10 @@ v2-prompts-by-brand-and-id:
tags:
- customer-config
summary: Update a single prompt
description: >-
`origin` in the body is not honored on PATCH — it is set once at create
time (see `V2PromptInput.origin`) and can never be changed afterward, so
any `origin` sent here is ignored.
operationId: updatePromptByBrandAndId
security:
- ims_key: [ ]
Expand Down
5 changes: 5 additions & 0 deletions docs/openapi/schemas.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -6643,6 +6643,11 @@ V2PromptInput:
type: string
enum: [ai, human]
default: human
description: >-
Who authored the prompt. Service-principal only: honored (and

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Item-6 confirmed: V2PromptInput.origin is KEPT (not deleted), now documented as service-principal-only + user-coerced-to-human. Retaining the field is what prevents relabeling DRS-written prompts. Doc matches the resolveOriginForWrite behavior.

validated against the enum) only when the caller authenticates as an
S2S service; for a user-authenticated request this field is ignored
and always overridden to `human`, regardless of what is sent.
source:
type: string
description: The source system or process that created the prompt
Expand Down
6 changes: 6 additions & 0 deletions src/controllers/brands.js
Original file line number Diff line number Diff line change
Expand Up @@ -554,6 +554,11 @@ function BrandsController(ctx, log, env) {

const { postgrestClient } = context.dataAccess.services;
const updatedBy = context.attributes?.authInfo?.profile?.email || 'system';
// Service principals (e.g. llmo-data-retrieval-service) authenticate over
// s2sAuthWrapper, which sets `context.s2sConsumer`; a user principal never
// carries it. Gates whether a per-prompt `origin` in the body is honored —
// see `resolveOriginForWrite`.
const isServicePrincipal = Boolean(context.s2sConsumer);

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

isServicePrincipal = Boolean(context.s2sConsumer) — verified s2sConsumer is the real attribute set by s2sAuthWrapper (same usage as llmo-mysticat-controller.js), so the detection is sound. But this exact line is where gate 5/6 needs a controller test: no case in test/controllers/brands.test.js sets context.s2sConsumer, so the TRUE branch (service principal honored) is never asserted at the endpoint. Please add the two controller cases (service→honored, user→coerced) here.


const brandUuid = await resolveBrandUuid(spaceCatId, brandId, postgrestClient);
if (!brandUuid) {
Expand All @@ -567,6 +572,7 @@ function BrandsController(ctx, log, env) {
postgrestClient,
updatedBy,
classifyIntent: classifyIntent ?? undefined,
isServicePrincipal,
});

return createResponse({ created, updated, prompts: outPrompts }, 201);
Expand Down
43 changes: 38 additions & 5 deletions src/support/prompts-storage.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,33 @@ import { classifyIntents } from './intent-classifier.js';
import { throwOnPgConstraintViolation } from './errors.js';
import { assertPermittedSource } from './prompt-sources.js';
import { INTENT_VALUES, normalizeIntent } from './intent.js';
import { ORIGIN_VALUE } from './serenity/prompt-tags.js';

const ORIGIN_VALUES = new Set(Object.values(ORIGIN_VALUE));

/**
* Resolves the `origin` a prompt write should carry, from the caller's
* authenticated principal rather than trusting the request body outright.
*
* A SERVICE principal (e.g. llmo-data-retrieval-service, which POSTs with a
* hardcoded `origin: "ai"`) may assert either enum value; an unrecognized
* value is treated as absent rather than rejected — a write must never fail
* over this field. A USER principal's asserted value is always ignored: a
* human hitting this endpoint is definitionally authoring the prompt
* themselves, so the body's `origin` (if any) is silently overridden, never
* used to reject the request.
*
* @param {*} candidate - the request body's asserted `origin`, if any.
* @param {boolean} isServicePrincipal - true when the caller authenticated as
* an S2S service (see `context.s2sConsumer` at the controller boundary).
* @returns {'ai' | 'human'}
*/
export function resolveOriginForWrite(candidate, isServicePrincipal) {

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

resolveOriginForWrite is correct and well-tested as a unit: service principal + valid enum → honored; service principal + unrecognized/absent → human (never throws); user principal → always human. The 'never reject over this field' property is important and upheld (unknown value coerces, doesn't 400). The gap is only that the CONTROLLER mapping feeding isServicePrincipal (from context.s2sConsumer) isn't tested end-to-end — see the gate 5/6 Should-Fix in the summary.

if (isServicePrincipal && ORIGIN_VALUES.has(candidate)) {
return candidate;
}
return ORIGIN_VALUE.HUMAN;
}

// Re-exported for backward compatibility — `normalizeIntent`/`INTENT_VALUES` now
// live in `./intent.js` so the LLM intent classifier can reuse them without an
Expand Down Expand Up @@ -406,7 +433,9 @@ function mapRowToPrompt(row) {
name: row.name,
regions: row.regions || [],
status: row.status || 'active',
origin: row.origin || 'human',
// No fallback: the column has zero NULLs in production, so `|| 'human'`
// was dead code that would otherwise silently mislabel a null as human.
origin: row.origin,
source: row.source || 'config',
intent: row.intent ?? null,
createdAt: row.created_at,
Expand Down Expand Up @@ -723,6 +752,9 @@ function buildPromptKey({ text, regions, source }) {
* text without an explicit intent. Non-fatal: a null result leaves intent unset.
* @param {number} [params.classifyIntentBatchTimeoutMs] - Cap on the classifier
* batch (ms); the upsert proceeds without intent once it elapses.
* @param {boolean} [params.isServicePrincipal] - true when the caller
* authenticated as an S2S service; gates whether a per-prompt `origin` in
* the request body is honored (see {@link resolveOriginForWrite}).
* @returns {Promise<{created: number, updated: number, prompts: object[]}>}
*/
export async function upsertPrompts({
Expand All @@ -733,6 +765,7 @@ export async function upsertPrompts({
updatedBy = 'system',
classifyIntent,
classifyIntentBatchTimeoutMs = 8000,
isServicePrincipal = false,
}) {
if (!postgrestClient?.from) {
throw new Error('PostgREST client is required for prompts');
Expand Down Expand Up @@ -825,7 +858,7 @@ export async function upsertPrompts({
category_id: categoryUuid,
topic_id: topicUuid,
status: p.status || 'active',
origin: p.origin || 'human',
origin: resolveOriginForWrite(p.origin, isServicePrincipal),
source,
intent: normalizeIntent(p.intent),
updated_by: updatedBy,
Expand Down Expand Up @@ -1050,9 +1083,9 @@ export async function updatePromptById({
if (updates.status !== undefined) {
patch.status = updates.status;
}
if (updates.origin !== undefined) {
patch.origin = updates.origin;
}
// `origin` is never patchable via the PATCH body — mirrors `source`, which is

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

UPDATE-preserve on the storage side: origin correctly dropped from the PATCH patch (mirrors source), so a PATCH body can never re-write origin. Combined with the injector's update-mode re-injection, origin is set once at create and never re-derived. Consistent with the OpenAPI note added to prompts-v2-api.yaml that PATCH origin is ignored.

// already absent here. It is set once at create time (from the authenticated
// principal — see `upsertPrompts`) and never re-derived or overridden later.
if (updates.intent !== undefined) {
// The shared fallback strips intent when the column is known-absent.
patch.intent = normalizeIntent(updates.intent);
Expand Down
2 changes: 1 addition & 1 deletion src/support/serenity/handlers/prompts-subworkspace.js
Original file line number Diff line number Diff line change
Expand Up @@ -294,7 +294,7 @@ export async function handleUpdatePromptSubworkspace(
const injectComputedType = makeTypeInjector(transport, workspaceId, classifyPromptType, log);
const typed = await injectComputedType(projectId, {
text: nextText, geoTargetId, tagIds: nextTagIds,
});
}, { mode: 'update' });

try {
await transport.renamePrompt(workspaceId, projectId, semrushPromptId, nextText);
Expand Down
130 changes: 93 additions & 37 deletions src/support/serenity/handlers/prompts.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,8 @@ import { redactUpstreamMessage } from '../rest-transport.js';
import { ERROR_CODES, isUpstreamGone } from '../errors.js';
import { normalizeGeoTargetId, normalizeLanguageCode, isValidTagIdFormat } from '../validation.js';
import { invalidateTagCacheForProject } from './markets.js';
import { resolveTypeValueInjection } from '../tag-tree.js';
import { resolveTypeValueInjection, resolveOriginValueInjection } from '../tag-tree.js';
import { ORIGIN_VALUE } from '../prompt-tags.js';

// TWIN FILE: the slice→project orchestration here is paralleled by the
// subworkspace-mode handlers in prompts-subworkspace.js. The duplication is
Expand Down Expand Up @@ -366,60 +367,115 @@ export async function createOnePrompt(transport, semrushWorkspaceId, projectId,
}

/**
* Builds the per-request `type` injector — the UNIFIED classification layer
* (serenity-docs#31). Given a pure `classifyPromptType(text, geoTargetId)`
* Builds the per-request `type` + `origin` injector — the UNIFIED
* classification layer (serenity-docs#31, extended by the-origin-dimension
* spec for `origin`). Given a pure `classifyPromptType(text, geoTargetId)`
* closure (built by the controller from the brand name + region-clamped
* aliases) that yields a BARE `type` value (`branded` / `non-branded`), it
* returns `injectComputedType(projectId, input)` which:
* - STRIPS every caller-supplied tag id that lives under the `type` root (the
* client may never set the value), and
* - APPENDS the pre-resolved upstream id of the server-computed value. The
* atomic `createPromptsByIds` 500s on an unresolved id, so it is resolved
* returns `injectComputedType(projectId, input, options)` which, for BOTH
* dimensions:
* - STRIPS every caller-supplied tag id that lives under the dimension's root
* (the client may never set either value directly), and
* - APPENDS a resolved upstream tag id in its place. The atomic
* `createPromptsByIds` 500s on an unresolved id, so ids are resolved
* BEFORE the write.
* The returned input carries the rewritten `tagIds`, so the caller's response
* echo reflects the computed type without a refetch (decision 5).
* echo reflects the computed values without a refetch (decision 5).
*
* The strip set is every id under the `type` root, not a name prefix: a tag's
* dimension is its root, and a sub-category could legitimately be named
* `branded` without being a `type` value.
* The strip set for each dimension is every id under that dimension's root,
* not a name prefix: a tag's dimension is its root, and a sub-category could
* legitimately be named `branded` or `ai` without being a `type`/`origin` value.
*
* Resolution ({@link resolveTypeValueInjection}, two tag-tree reads per distinct
* `type` value per project — the root level plus the `type` root's children) is
* memoized for the request, so a bulk create fans out over the distinct computed
* values rather than over the items. A non-function `classifyPromptType`
* (defensive) is a pass-through.
* `type` is always freshly classified from the write's own text. `origin` is
* asymmetric between create and update (`options.mode`, default `'create'`):
* - `'create'`: derives to {@link ORIGIN_VALUE.AI} — every prompt this layer
* creates is AI-generated.
* - `'update'`: re-injects whichever `origin` tag id is ALREADY in
* `input.tagIds` (the client round-trips the tags it read off the prior
* list call), rather than deriving a new one from anything in the update
* request. This is deliberate: `origin` records who authored the prompt
* ORIGINALLY, which an edit does not change. If none is found (a prompt
* tagged before this dimension existed), it falls back to `AI` rather than
* leave the prompt untagged — a strip-without-inject would make it invisible
* to origin-based filtering.
*
* `resolveTypeValueInjection` resolves or throws, so the computed tag is always
* attached. It must never be dropped: `type` is the one dimension a client may
* not set, so a prompt written without it stays unclassified forever, and the
* caller sees a 2xx. Failing the write instead is free — the upstream bulk create
* is atomic and has not run yet.
* Resolution (two tag-tree reads per distinct value per project — the root
* level plus that dimension root's children) is memoized for the request per
* dimension, so a bulk create fans out over the distinct computed values
* rather than over the items. A non-function `classifyPromptType` (defensive)
* skips the `type` step; `origin` is always injected.
*
* Both resolvers resolve or throw, so the computed tags are always attached.
* They must never be dropped: neither dimension is client-settable, so a
* prompt written without one stays unclassified forever, and the caller sees a
* 2xx. Failing the write instead is free — the upstream bulk create is atomic
* and has not run yet.
*
* @param {object} transport - Serenity transport (Semrush proxy client).
* @param {string} semrushWorkspaceId
* @param {((text: string, geoTargetId: number) => string) | undefined} classifyPromptType
* @param {object} [log]
* @returns {(projectId: string, input: { text: string, geoTargetId: number,
* tagIds: string[] }) =>
* tagIds: string[] }, options?: { mode?: 'create' | 'update' }) =>
* Promise<{ text: string, geoTargetId: number, tagIds: string[] }>}
*/
export function makeTypeInjector(transport, semrushWorkspaceId, classifyPromptType, log) {
/** @type {Map<string, Promise<{ computedId: string, typeTagIds: string[] }>>} */
const cache = new Map();
return async function injectComputedType(projectId, input) {
if (typeof classifyPromptType !== 'function') {
return input;
const typeCache = new Map();
/** @type {Map<string, Promise<{ computedId: string, originTagIds: string[] }>>} */
const originCache = new Map();

return async function injectComputedType(projectId, input, { mode = 'create' } = {}) {
let out = input;

if (typeof classifyPromptType === 'function') {
const typeValue = classifyPromptType(input.text, input.geoTargetId);
const typeKey = `${projectId} ${typeValue}`;
let typePending = typeCache.get(typeKey);
if (!typePending) {
typePending = resolveTypeValueInjection(
transport,
semrushWorkspaceId,
projectId,
typeValue,
log,
);
typeCache.set(typeKey, typePending);
}
const { computedId, typeTagIds } = await typePending;
const stripped = out.tagIds.filter((id) => !typeTagIds.includes(id));
out = { ...out, tagIds: [...stripped, computedId] };
}
const typeValue = classifyPromptType(input.text, input.geoTargetId);
const key = `${projectId} ${typeValue}`;
let pending = cache.get(key);
if (!pending) {
pending = resolveTypeValueInjection(transport, semrushWorkspaceId, projectId, typeValue, log);
cache.set(key, pending);

// `origin` resolution is the same tree lookup on both create and update —
// every id under the root, plus the resolved `AI` id — only what CREATE
// does with it differs from what UPDATE does.
const originKey = `${projectId} ${ORIGIN_VALUE.AI}`;
let originPending = originCache.get(originKey);
if (!originPending) {
originPending = resolveOriginValueInjection(
transport,
semrushWorkspaceId,
projectId,
ORIGIN_VALUE.AI,
log,
);
originCache.set(originKey, originPending);
}
const { computedId, typeTagIds } = await pending;
const stripped = input.tagIds.filter((id) => !typeTagIds.includes(id));
return { ...input, tagIds: [...stripped, computedId] };
const { computedId: aiId, originTagIds } = await originPending;
const stripped = out.tagIds.filter((id) => !originTagIds.includes(id));

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Strip-by-id confirmed: filters against originTagIds (the ids under the authorship root via [...byName.values()]), never by name — so a sub-category legitimately named ai/human under a different root is untouched. Matches the type strip and the lens requirement.


// UPDATE re-injects whichever origin tag the caller already carried — it
// never derives a new one. CREATE always derives to AI. Falling back to AI
// when update finds none avoids a strip-without-inject (which would make
// the prompt invisible to origin-based filtering) for a prompt tagged
// before this dimension existed.
const nextOriginId = mode === 'update'

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

This is the create/update asymmetry, and it's correct: UPDATE re-injects the origin id the caller already carries (out.tagIds.find((id) => originTagIds.includes(id))) and only falls back to aiId when none is present; CREATE always uses aiId. Because originTagIds is every id under the resolved root, the preceding stripped filter removes any prior origin id before this re-appends exactly one — so an edit that round-trips originHuman keeps human, never relabels to ai. Verified by the new preserves an existing human origin on edit test. The fallback-to-ai for a pre-dimension prompt (avoiding strip-without-inject) is the right call — a stripped-but-uninjected prompt would drop out of origin-filtered reads.

? (out.tagIds.find((id) => originTagIds.includes(id)) ?? aiId)
: aiId;
out = { ...out, tagIds: [...stripped, nextOriginId] };

return out;
};
}

Expand Down Expand Up @@ -722,7 +778,7 @@ export async function handleUpdatePrompt(
);
const typed = await injectComputedType(projectId, {
text: nextText, geoTargetId, tagIds: nextTagIds,
});
}, { mode: 'update' });

try {
await transport.renamePrompt(semrushWorkspaceId, projectId, semrushPromptId, nextText);
Expand Down
43 changes: 43 additions & 0 deletions src/support/serenity/tag-tree.js
Original file line number Diff line number Diff line change
Expand Up @@ -608,3 +608,46 @@ export async function resolveTypeValueInjection(
typeTagIds: [...byName.values()],
};
}

/**
* Resolves the id-based injection of a server-computed `origin` value into a
* prompt write. Returns the wanted value's id plus EVERY id under the resolved
* authorship root, so the caller can strip any caller-supplied `origin` tag id
* (a client must never set the value itself on create; on update the caller
* re-injects the prompt's already-stored id instead of calling this with a
* freshly derived one — see `makeTypeInjector` in `handlers/prompts.js`).
*
* The root is resolved through {@link ensureDimensionRoots}, so during the
* `source` → `origin` rename this returns the tolerantly-resolved authorship
* root (whichever physical name it currently carries) — never a second one.
*
* @param {object} transport - Serenity transport (Semrush proxy client).
* @param {string} semrushWorkspaceId
* @param {string} projectId
* @param {string} wantValue - the bare `origin` value (`ai` / `human`).
* @param {object} [log] - logger.
* @returns {Promise<{ computedId: string, originTagIds: string[] }>} `computedId`
* is always resolved — {@link ensureChildren} throws rather than leave a hole.
*/
export async function resolveOriginValueInjection(

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

resolveOriginValueInjection resolves the root through ensureDimensionRoots, so it inherits O2a's tolerant origin/legacy-source resolution — the returned originTagIds/computedId reference whichever physical authorship root the project currently carries, and it never mints a second one. Correct dependency on the O2a seam, and the docstring calls this out.

transport,
semrushWorkspaceId,
projectId,
wantValue,
log,
) {
const roots = await ensureDimensionRoots(transport, semrushWorkspaceId, projectId, log);
const originRootId = rootIdOf(roots, DIMENSION.ORIGIN);
const { byName } = await ensureChildren(
transport,
semrushWorkspaceId,
projectId,
originRootId,
[wantValue],
log,
);
return {
computedId: /** @type {string} */ (byName.get(wantValue)),
originTagIds: [...byName.values()],
};
}
9 changes: 5 additions & 4 deletions test/it/shared/tests/serenity.js
Original file line number Diff line number Diff line change
Expand Up @@ -533,11 +533,12 @@ export default function serenityTests(
expect(created.status).to.equal(200);
expect(created.body.created).to.have.lengthOf(1);
expect(created.body.created[0].semrushPromptId).to.be.a('string').that.is.not.empty;
// The write path now server-computes a branded/non-branded `type:` tag and
// appends it to the supplied tagIds, so the created prompt carries the two
// supplied tags plus one computed type tag.
// The write path server-computes two tags and appends them to the supplied
// tagIds: a branded/non-branded `type` tag and an `origin` tag (`ai` on
// create — WP-O2b). So the created prompt carries the two supplied tags
// plus the computed type and origin tags.
expect(created.body.created[0].tagIds).to.include.members([category.body.id, child.body.id]);
expect(created.body.created[0].tagIds).to.have.lengthOf(3);
expect(created.body.created[0].tagIds).to.have.lengthOf(4);
expect(created.body.failed).to.deep.equal([]);

// by_tags correlation: the id-based create embeds the tag ids, so filtering the prompt list
Expand Down
Loading
Loading