feat(serenity): server-owned source dimension + canonicalizeSource (LLMO-6282) [WP-S2]#2867
feat(serenity): server-owned source dimension + canonicalizeSource (LLMO-6282) [WP-S2]#2867aliciadriani wants to merge 8 commits into
Conversation
|
This PR will trigger a minor release when merged. |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
aliciadriani
left a comment
aliciadriani
left a comment
There was a problem hiding this comment.
Reviewed WP-S2 (LLMO-6282): server-owned source dimension, canonicalizeSource, and the SERVER_OWNED_DIMENSIONS refactor. Base claude/wp-o2b-derivation (stacked, non-main) → MysticatBot N/A. DO-NOT-MERGE banner + WP-O6 gate expected. it-postgres red is the SQL sort/filter that pairs with WP-S4's index (it/-gated, unit-only now) — not a finding per the lens.
Audited each lens item; the design is strong. One defense-in-depth Should-Fix.
Headline refactor — CONFIRMED correct. I audited each former isClosedDimension() call site for which question it was actually asking:
parseCreateTagBody: the parentId write-guard now keys onisServerOwnedDimension(line 240) — sosourcecorrectly forbids aparentId(its values hang off the root) even though it's open; the name enum-check stays onisClosed(closedValuesOf, line 222) — sosourceis correctly NOT enum-checked (open vocabulary). That's exactly the two-questions split the WP intends.handleCreateTag/handleCreateTagSubworkspace: the resolve-or-create branch flipsisClosed→isServerOwned, routingsourcethroughensureServerOwnedValue(the renamed, vocabulary-agnosticensureClosedValue). Correct — resolve-or-create is the server-owned semantic, and the enum enforcement was already lifted to the caller.
isClosedDimensionis left with its one honest job (vocabulary), and no other call site remained mis-asking.
canonicalizeSource — CONFIRMED. value.trim().toLowerCase().replace(/_/g,'-'); returns null (never a default) on non-string / empty-after-trim / : / >MAX_TAG_NAME_LEN / isDimensionRootName. This is the reference twin the CLI Python side is being aligned to (the CLI's missing .strip() was my WP-S4 finding). SOURCE_LABEL is frozen and its exhaustiveness is enforced by a CI test (prompt-tags.test.js: Object.keys(SOURCE_LABEL).sort() deep-equals SOURCE_VALUES, fails on any label-less value) with no ?? slug pass-through default — matches §7.
Distinctness guard — CONFIRMED. ensureDimensionRoots classifies the single physical source root once: if it's authorship (children ⊆ {ai,human}, no origin) the map's source key is undefined and the producer root is neither created nor returned; it resolves only on a fresh project (created outright) or a post-rename project (authorship moved to origin). The producing-system root is never conflated with a mid-rename authorship root — which is exactly why the PR is WP-O6-gated.
Item 13 — CONFIRMED. V2Prompt.source is readOnly: true (canonical form); source is REMOVED from V2PromptInput; the v2 list gains a source query filter + a source sort enum (both server-side canonical-fold, lower(replace(source,'_','-'))); create-tag type enum gains source. Bodies can't set it.
Origin/source write-side — CONFIRMED. mapRowToPrompt → canonicalizeSource(row.source) ?? row.source ?? 'config' (2nd boundary; RAW string on guard failure, not null — right call for the grid); deriveV2PromptOrigin from the principal, body dropped in brands.js ({ source: _, ...p }); upsertPrompts preserves stored source AND origin on update; updatePromptById patches neither; write-side || 'config' default kept for internal writers.
SHOULD-FIX (defense-in-depth; CONFIRMED mechanism, reachability gated by WP-O6) — the mid-rename source path can mint a ROOT-LEVEL config tag, silently, if the gate is ever bypassed
The distinctness guard in ensureDimensionRoots correctly returns source: undefined on a mid-rename project, and rootIdOf's own docstring warns "the source root can be undefined… callers that need it read roots.get(DIMENSION.SOURCE) directly." But the source-injection path does NOT heed that: makePromptTagInjector (called on EVERY handleCreatePrompts create with sourceValue: 'config') → resolveClosedValueInjection(transport, ws, pid, DIMENSION.SOURCE, 'config') → rootIdOf(roots, SOURCE), and rootIdOf is just roots.get(dimension) with no guard, so on a mid-rename project it returns undefined. That undefined flows into ensureChildren(parentId=undefined, ['config']), whose create is createProjectTags(missing, parentId ? { parentId } : {}) — with parentId undefined this becomes {}, a ROOT-LEVEL create: it mints a bare config root tag and tags the prompt with it. Not a fail-loud abort — a silent stranded-root corruption, precisely what the dimension-root model forbids.
This is fully mitigated by the EXTERNAL WP-O6 deployment gate (post-WP-O6 every project has the source name freed, so roots.get(SOURCE) is always defined) plus the DO-NOT-MERGE banner — so it is not a live bug on the intended timeline, and I'm not blocking on it. But there is no in-code guard: nothing stops the source-injection path from passing an undefined root id to ensureChildren if this ever ran on a not-yet-renamed project (a premature deploy, or a project that slips past the gate). Recommend a cheap guard that makes the design's stated invariant ("MUST NOT be provisioned before WP-O6") fail LOUD rather than corrupt: in the source injection branch (or resolveClosedValueInjection when dimension === DIMENSION.SOURCE, or ensureChildren when parentId is undefined vs ''), throw a clear "source dimension not provisioned (WP-O6-gated)" error instead of a root-level create. I couldn't find a test exercising source-injection on a mid-rename project (the passing suite covers fresh/post-rename, where the root resolves), so this path is currently unguarded and untested. Routing to the implementer to decide guard-vs-document.
No Must-Fix. /review-pr otherwise clean; the above is a hardening request against a gate-bypass, not a mainline defect. Bot N/A (non-main base).
| * is in a dialog to resolve it first. | ||
| * `isClosedDimension` keeps its one honest job: vocabulary validation. | ||
| */ | ||
| export const SERVER_OWNED_DIMENSIONS = Object.freeze([ |
There was a problem hiding this comment.
SERVER_OWNED_DIMENSIONS = [intent, origin, type, source] is the right axis, cleanly separated from open/closed. The docstring nails why: two independent properties (vocabulary open/closed vs writer customer/server), and source is the open-AND-server-owned cell. Verified both decisions route through this (write-guard + create-semantics) while isClosedDimension keeps only vocabulary validation.
| } | ||
| const parentId = parseParentId(body?.parentId); | ||
| if (isClosed && parentId !== undefined) { | ||
| if (isServerOwned && parentId !== undefined) { |
There was a problem hiding this comment.
Call-site audit confirmed correct: the parentId write-guard keys on isServerOwned here (so source forbids a parentId — its values hang off the root), while the name enum-check a few lines up (line 222, isClosed && !closedValuesOf(type).includes(rawName)) stays on isClosed (so source is NOT enum-checked — open vocabulary). That is exactly the two-questions split: source = server-owned (guarded) + open (not enum-checked).
| * @param {unknown} value - a raw `prompts.source` value. | ||
| * @returns {string | null} the canonical slug, or `null` when it must not be tagged. | ||
| */ | ||
| export function canonicalizeSource(value) { |
There was a problem hiding this comment.
canonicalizeSource confirmed as the reference twin: value.trim().toLowerCase().replace(/_/g,'-'), null (never a default) on non-string / empty / : / >MAX_TAG_NAME_LEN / isDimensionRootName. The .trim() here is the semantics the migration CLI's Python twin was missing (my WP-S4 finding, being fixed there). Good single-source-of-truth with MAX_TAG_NAME_LEN.
| * and nowhere else. (elmo ships its own display labels behind `SOURCE_BADGE_CONFIG`; | ||
| * WP-S3.) | ||
| */ | ||
| export const SOURCE_LABEL = Object.freeze( |
There was a problem hiding this comment.
SOURCE_LABEL frozen + exhaustive with NO ?? slug pass-through default, and the CI test (prompt-tags.test.js line ~166) fails the moment a SOURCE_VALUES entry lacks a label. This is the right shape for §7 — an internal slug can't silently reach a customer, and the label decision lands in exactly one place.
| authorshipId = legacyId; | ||
| } | ||
| let legacySourceIsAuthorship = false; | ||
| const physicalSourceIsAuthorship = !authorshipId && physicalSourceId |
There was a problem hiding this comment.
Distinctness guard is correct: the single physical source root is classified once (physicalSourceIsAuthorship), and the producer source root is created/returned only when the name is free (fresh or post-rename), else the map's source key is undefined. Never conflates the producing-system root with a mid-rename authorship root — the WP-O6-gate rationale, verified.
| * @param {Map<string, string>} roots - the resolved root name → id map. | ||
| * @param {string} dimension - one of the four dimension root names. | ||
| * @param {Map<string, string | undefined>} roots - the resolved root name → id map. | ||
| * @param {string} dimension - one of the always-resolved dimension root names. |
There was a problem hiding this comment.
SHOULD-FIX (defense-in-depth): this docstring correctly warns the source root can be undefined and that callers needing it should read roots.get(DIMENSION.SOURCE) directly — but resolveClosedValueInjection (line 652) does NOT: it calls rootIdOf(roots, DIMENSION.SOURCE) (which is a bare roots.get, no guard), so on a mid-rename project it hands undefined to ensureChildren, whose createProjectTags(missing, parentId ? {parentId} : {}) then creates the value at ROOT level (a stranded config root). Gated externally by WP-O6, but unguarded and untested in code. Consider throwing a clear "source not provisioned (WP-O6-gated)" when dimension === DIMENSION.SOURCE and the root id is undefined, so a gate-bypass fails loud instead of corrupting.
| // source — CREATE only, same asymmetry as origin. `sourceValue` unset means | ||
| // UPDATE: leave the producer alone (fixed at creation). Stripped by resolved id | ||
| // beneath the `source` root, never by name (a category may be `gsc`). | ||
| if (sourceValue) { |
There was a problem hiding this comment.
The source injection branch (CREATE-only, memoized, stripped by resolved root id not by name) is correct on a fresh/post-rename project. It's the entry point of the mid-rename concern flagged on tag-tree.js:357 — this unconditionally calls resolveClosedValueInjection(DIMENSION.SOURCE, sourceValue) whenever sourceValue is set (always, on handleCreatePrompts), with no check that roots.get(SOURCE) resolved. The origin branch above is safe (origin resolves tolerantly to the legacy root); only source can be undefined mid-rename.
| // write-side default stays load-bearing for the internal writers that DO set | ||
| // it. `updatePromptById` likewise never patches source (producer is fixed at | ||
| // creation). | ||
| const derivedPrompts = prompts.map(({ source: _, ...p }) => ({ |
There was a problem hiding this comment.
{ source: _, ...p } correctly drops any body source (no write surface) and deriveV2PromptOrigin derives origin from the principal (user→human, service→believed). Confirmed the drop makes a v2 create fall to the store's config default, and the write-side || 'config' in upsertPrompts stays load-bearing for internal writers that DO set source. Clean item-5/6 handling.
| // (empty, `:`, over-long, root-shadowing) returns the RAW string rather than | ||
| // null: the grid must still show the operator what is stored. `?? 'config'` only | ||
| // guards a nullish column (in-memory/test rows); the DB column is NOT NULL. | ||
| source: canonicalizeSource(row.source) ?? row.source ?? 'config', |
There was a problem hiding this comment.
Second derivation boundary correct: canonicalizeSource(row.source) ?? row.source ?? 'config' returns the canonical slug, falls back to the RAW string on a guard failure (so the grid still shows what's stored — deliberately not null here, unlike the tag-write boundary), and ?? 'config' only guards a nullish column. origin: row.origin with no || 'human' fallback is also right (NOT NULL in prod; a fallback would mislabel).
4d66cd6 to
b6f5fe2
Compare
aliciadriani
left a comment
aliciadriani
left a comment
There was a problem hiding this comment.
Re-check of WP-S2 at 8758de5a — my Should-Fix (unguarded mid-rename source root id degrading into a root-level create) is FIXED. S2 is now review-clean. Base non-main → bot N/A.
The fix adds requireServerOwnedRootId(roots, dimension) — throws ErrorWithStatusCode(..., 502) with a clear message ("<dim> dimension root not provisioned (source-dimension.md is WP-O6-gated); refusing to create a stranded root-level tag") when the resolved server-owned root id is falsy — and swaps it in for the bare rootIdOf at BOTH sites I named: resolveClosedValueInjection (the create-path injector) and ensureServerOwnedValue (create-tag + the semrush-stamp generator). Confirming the two requested points:
(a) No-op for always-provisioned dimensions — CONFIRMED. requireServerOwnedRootId returns the id unchanged when it's truthy, throwing only on a falsy id. intent / origin / type are always resolved by ensureDimensionRoots (created when missing; origin resolves tolerantly to a real physical root), so their root ids are never falsy and the guard is a pass-through — behaviour for those dimensions is identical to before. Only source on a mid-rename project can be undefined (the deliberate distinctness-guard gap), so only that case changes.
(b) Mid-rename source now THROWS with zero root-level create — CONFIRMED. The new tests pin it exactly:
tag-tree.test.js: on a mid-rename tree (legacysourceroot with ai/human, noorigin),ensureDimensionRootsleavesroots.get('source')undefined;ensureServerOwnedValue(source, 'config')andresolveClosedValueInjection(source, 'config')each reject withstatus===502, message/source dimension root not provisioned/, andcreateProjectTagsis NOT called — so no stranded root-levelconfigtag is minted.prompts.test.js: the handler-level path —handleCreatePromptson a mid-rename project — puts the item infailedwithstatus===502, calls neithercreatePromptsByIdsnorcreateProjectTags. Fail-loud, nothing written, no corruption.
The guard turns the design's stated invariant ("source MUST NOT be provisioned before WP-O6") from a silent tree-corruption-on-gate-bypass into a visible 502, which is exactly what I asked for. ensureDimensionRoots's distinctness guard (returning source: undefined mid-rename) is unchanged and still correct; the fix is purely the downstream consumer hardening.
I could not run the 15305-unit suite locally (no Python/JS worktree with deps here), so I relied on the reported pass count for the empirical confirmation — the change is small, surgical, and the added tests cover the exact scenarios. All other WP-S2 lens items remain as CONFIRMED in my prior review (SERVER_OWNED_DIMENSIONS split, canonicalizeSource trim + SOURCE_LABEL exhaustiveness, distinctness guard, item 13, write-side origin/source). Net: /review-pr clean. Bot N/A (non-main base).
8758de5 to
a9539ee
Compare
aliciadriani
left a comment
aliciadriani
left a comment
There was a problem hiding this comment.
Re-check at a9539eec (rebased onto O2b's advanced head, now MERGEABLE, 15346 unit pass). Verified the single rebase conflict resolution in brands.js createPromptsByBrand and the auto-merged O2b-touched files. WP-S2 remains review-clean. Base non-main → bot N/A.
brands.js conflict — BOTH changes intact, no regression:
- O2b auth-hardening (MysticatBot):
const { authInfo } = context.attributes ?? {}→authType = typeof authInfo?.getType === 'function' ? authInfo.getType() : undefined→isUserPrincipal = !authType || authType === 'jwt' || authType === 'ims'. Fail-safe to the least-privileged USER principal on absent/indeterminate auth (!authType → user; a non-functiongetType→undefined→ user), soorigincan't be spoofed by an unauthenticated/odd caller. - S2 body-
sourcestrip: the prompt mapping isprompts.map(({ source: _, ...p }) => ({ ...p, origin: deriveV2PromptOrigin(p?.origin, isUserPrincipal) }))— a body-suppliedsourceis destructured out and never reachesupsertPrompts, andoriginis principal-derived through the hardenedisUserPrincipal. - The two compose orthogonally in one
.map(auth-hardening feedsisUserPrincipal; the strip dropssource): origin stays principal-derived + user-fail-safe, source stays server-owned/body-ignored. No regression.
Auto-merged O2b files consistent with S2's source dimension:
prompts-storage.jsmapRowToPromptkeeps BOTH O2b'sorigin: row.origin(no|| 'human') and S2'ssource: canonicalizeSource(row.source) ?? row.source ?? 'config'; the S2SOURCE_CANONICAL_SQLsort/filter andderiveV2PromptOriginare all present.prompts.jsmakePromptTagInjectorthreads BOTHoriginValue(O2b) andsourceValue(S2) viaconst { originValue, sourceValue } = options.markets-subworkspace.jsretains S2's producing-system stamp (ensureServerOwnedValue(..., DIMENSION.SOURCE, GENERATED_PROMPT_SOURCE_VALUE),sourceIdappended to the generated prompts' tag ids).tag-tree.jsstill carries my S2 guard fixrequireServerOwnedRootId(defined + used at bothresolveClosedValueInjectionandensureServerOwnedValue), so the mid-renamesource-root fail-loud behavior survived the rebase.
Green unit run + MERGEABLE confirm the merge is sound at the compile/test level; the spot-checks confirm no S2 change was clobbered by an O2b change or vice versa. /review-pr clean. Merge ≥ O6. Bot N/A (non-main base).
|
The change at const derivedPrompts = prompts.map(({ source: _, ...p }) => ({
...p,
origin: deriveV2PromptOrigin(p?.origin, isUserPrincipal),
}));The comment justifies this by saying the That's a problem, because the Strategic Recommendations "Track" flow in
Those four values are exactly Concrete impact: Semrush-tracked prompts lose their provenance and collapse to Suggested resolution — pick one before merge:
Either way, worth a quick sync with the SITES-47870 owner so the |
aliciadriani
left a comment
aliciadriani
left a comment
There was a problem hiding this comment.
Automated /review-pr pass (WP-S2). One verified merge-blocker on the source filter/sort mechanism, plus one drift nit. MysticatBot step skipped — this PR's base is claude/wp-o2b-derivation, not main, and the org bot only reviews main-based PRs. Details inline.
| // folded with the same transform as the column, and matched against the SQL | ||
| // expression rather than the raw column. | ||
| const wantedSource = String(source).trim().toLowerCase().replace(/_/g, '-'); | ||
| baseQuery = baseQuery.eq(SOURCE_CANONICAL_SQL, wantedSource); |
There was a problem hiding this comment.
Must fix (verified) — this filter cannot work against stock PostgREST.
SOURCE_CANONICAL_SQL (lower(replace(source, '_', '-'))) is passed as the column to .eq() here, and to .order() on line 633. I confirmed the backend (mysticat-data-service) is stock PostgREST 12 and that there is no source_canonical computed column/function on prompts. postgrest-js emits ?lower(replace(source,'_','-'))=eq.<v> and order=lower(replace(source,'_','-')).asc, which stock PostgREST rejects with a 400 (PGRST100) — filter keys and order accept only column names / computed columns / JSON paths, never inline function expressions.
The current unit tests pass only because the mock records the column string without validating it against PostgREST semantics, so ?source=… and sort=source would 400 in production.
Suggested fix: add a source_canonical GENERATED ALWAYS AS (lower(replace(source,'_','-'))) STORED column in mysticat-data-service (fold into WP-S4 next to idx_prompts_source_canonical, and index that column), then filter/sort on the source_canonical column name here. Alternatively drop drift-folding on filter/sort and match the raw source column. An integration test against real PostgREST would catch this whole class of bug.
| if (sortCol.includes('(')) { | ||
| if (typeof sortCol === 'object') { | ||
| // A SQL expression (e.g. the canonical `source` fold), ordered directly. | ||
| baseQuery = baseQuery.order(sortCol.expression, { ascending }); |
There was a problem hiding this comment.
Same must-fix as the filter on line 666: .order() with the raw expression lower(replace(source, '_', '-')) produces order=lower(replace(source,'_','-')).asc, which stock PostgREST cannot parse (400). See the detailed comment on line 666 for the suggested source_canonical generated-column fix.
| // `citation_attempt` too (source-dimension.md §3.1). The incoming value is | ||
| // folded with the same transform as the column, and matched against the SQL | ||
| // expression rather than the raw column. | ||
| const wantedSource = String(source).trim().toLowerCase().replace(/_/g, '-'); |
There was a problem hiding this comment.
Nit: this inline fold is a third copy of the canonical transform (alongside canonicalizeSource and SOURCE_CANONICAL_SQL — this file already warns "the two must not drift"). Once the filter/sort mechanism is settled (see line 666), route the incoming value through a single shared helper — reuse canonicalizeSource, or extract a foldSource(value) — so the copies can't drift.
aliciadriani
left a comment
aliciadriani
left a comment
There was a problem hiding this comment.
Applied the WP-S2 interim fix for the PostgREST filter/sort blocker (commit a681496). Filter/sort now run on the raw source column; the canonical-column fold is deferred to WP-S4. Inline notes on the fixed lines below.
| // matching drops). | ||
| const wanted = foldSourceValue(source); | ||
| const sourceVariants = [...new Set([wanted, wanted.replace(/-/g, '_')])]; | ||
| baseQuery = baseQuery.in('source', sourceVariants); |
There was a problem hiding this comment.
Fixed (must-fix). Was .eq("lower(replace(source,'_','-'))", …) — an inline SQL expression stock PostgREST 400s on. Now matches the raw source column across both drift spellings via .in(), so citation-attempt still finds citation_attempt rows. WP-S4 replaces this with a single source_canonical generated-column match.
| // index) and ordering moves to that column name, so the two drift spellings order | ||
| // together; until then raw ordering only interleaves the `_`/`-` spellings of the | ||
| // same producer, which is cosmetic (source-dimension.md §3.1). | ||
| source: 'source', |
There was a problem hiding this comment.
Fixed (must-fix). Sort key was an object { expression: "lower(replace(...))" } PostgREST can't order by (400). Now the raw source column; the object-form dispatch branch was removed. Canonical ordering returns with WP-S4's source_canonical column.
| * @param {string} value - a `source` value (already known to be a string). | ||
| * @returns {string} the folded value. | ||
| */ | ||
| export function foldSourceValue(value) { |
There was a problem hiding this comment.
Fixed (nit). Extracted foldSourceValue as the single definition of the trim/lower/_→- transform. canonicalizeSource and the v2 list source filter now both route through it, so the copies that previously risked drift are collapsed to one.
aliciadriani
left a comment
aliciadriani
left a comment
There was a problem hiding this comment.
Iteration 2 (/review-pr on the updated PR). The must-fix and nit from iteration 1 are resolved; one self-review finding (a robustness regression I introduced) is fixed below. No remaining must/should/nits. 940 tests pass, lint clean, coverage 100% lines/funcs · 98.6% branches (>97%).
| * @returns {string} the folded value. | ||
| */ | ||
| export function foldSourceValue(value) { | ||
| return String(value).trim().toLowerCase().replace(/_/g, '-'); |
There was a problem hiding this comment.
Fixed (self-review). Routing the v2 source filter through this shared helper dropped the defensive String() the inline fold had — a non-string query param (parsed as a number/array) would hit value.trim() and 500. Restored the String() coercion here; canonicalizeSource type-guards before calling, so it is a no-op on that path.
…LMO-6275) (#2866) ## WP-O2b (LLMO-6275) — origin derivation + injector generalization **Canonical WP-O2b implementation** (serenity dimension-root program, epic 46). Stacked on `claude/wp-o2a-origin`. Generalizes `makeTypeInjector` → `makePromptTagInjector` to strip-and-inject both `type` and `origin` with the spec §3 item-3 asymmetry (CREATE injects the derived value; UPDATE re-injects the prompt's STORED origin, never re-derives), strips by resolved tag id beneath the root (never by name), derives `origin` from the request principal (service principal → asserted body value validated against the vocabulary; user principal → `human`, body ignored, never rejected), stops patching `origin` on update, `mapRowToPrompt` returns `row.origin` (no `|| 'human'` fallback), and KEEPS `origin` on `V2PromptInput` (the DRS `origin:'ai'` upsert guard, item 6). Spec gates 5–8, including the **gate5/6 controller coverage** in `test/controllers/brands.test.js`. Verify: node24 type-check + lint + npm test (unit) green. `it/`-postgres waits the WP-O1 mock release (not a finding). **Supersedes #2864** (the earlier "salvage" implementation from #2825), which lacked the gate5/6 controller coverage. This branch is also the confirmed base for WP-S2 (#2867). History preserved in #2864. --- ## WP-O2b — derive & inject the `origin` dimension server-side The **derivation** half of WP-O2 in the origin rename ([serenity-docs#46](adobe/serenity-docs#46)). Tracks **LLMO-6275** (epic LLMO-6270). > **Stacked on WP-O2a.** Base is `claude/wp-o2a-origin` ([#2862](#2862)), so this diff is *only* the derivation — it builds on O2a's `DIMENSION.ORIGIN`, `ORIGIN_VALUE`, `STANDARD_PROMPT_TAG_VALUES`, and the tolerant `ensureDimensionRoots` resolver. Review/merge O2a first. **Draft — do not merge.** `origin` (who authored a prompt's text: `human` / `ai`) becomes a function of the **write path**, never a caller-supplied field, with the create/update asymmetry [origin-dimension.md §3](adobe/serenity-docs#46) requires. ### Serenity tag path — `handlers/prompts.js` + `prompts-subworkspace.js` - `makeTypeInjector` → **`makePromptTagInjector`**, now stamping both `type` and `origin`: - **CREATE** strips any caller-supplied tag id beneath the `origin` root (by **resolved id, never by name** — a customer category named `ai` survives) and injects the derived `human`. - **UPDATE** leaves `origin` alone — the stored value the caller echoes rides through the replace-mode write, **never re-derived** (re-deriving would relabel an edited `ai` prompt `human`; stripping-without-injecting would make it invisible — both illegal). - `resolveClosedValueInjection` generalizes `resolveTypeValueInjection` (the origin root is resolved tolerantly, so it addresses whichever physical root — `origin` or a legacy `source` — the project carries). ### v2 prompts API — `controllers/brands.js` + `prompts-storage.js` - `createPromptsByBrand` derives `origin` from the **principal**: an IMS/JWT **user** → `human` (body ignored, never rejected); a **service principal** (admin `x-api-key`, e.g. DRS) → validated body value — the DRS `origin:'ai'` contract stays honoured. - `upsertPrompts` preserves the stored `origin` on a match-update; `updatePromptById` no longer patches `origin`; `mapRowToPrompt` returns `row.origin` verbatim (no `|| 'human'`). - OpenAPI: `origin` documented as service-principal-only / read-only for users and not patchable on update; the `origin` query filter + sort enum stay. The user-facing elmo write surface (Source picker, CSV import) is **WP-O3b**, a separate WP — not here. ### Gates ([origin-dimension.md §7](adobe/serenity-docs#46)) - **5** (a user cannot write origin) — v2 arm + Serenity create arm ✅ - **6** (a service principal can) — DRS `origin:'ai'` honoured ✅ - **7** (editing does not relabel) — an `ai` prompt stays `ai` across an edit ✅ - **8** (strip by id) — a category named `ai` survives a create carrying an origin id ✅ ### Verification - `npm test` **exit 0** — 15285 passing, coverage thresholds met. - `npm run type-check` clean; `eslint` clean on all touched paths; `docs:lint` valid (only pre-existing warnings, incl. the unrelated `/configurations/` ambiguity). - `test/it/` integration is **out of scope** — it needs WP-O1 ([spacecat-shared#1831](adobe/spacecat-shared#1831)) released for the PE-client mock image; not run here. --------- Co-authored-by: Alicia Adriani <aadriani+adobe@adobe.com> Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
aliciadriani
left a comment
aliciadriani
left a comment
There was a problem hiding this comment.
Iteration 3 (/review-pr after merging base claude/wp-o2b-derivation). Merge conflict resolved — PR is now MERGEABLE. Full suite 15396 passing (exit 0, coverage gate met), lint clean. Two conflicts, both resolved by combining rather than choosing a side; notes inline. No new must/should fixes — the WP-S2 changes are unchanged from the reviewed state, and the base merge introduced no regressions.
| workspaceId, | ||
| projectId, | ||
| items, | ||
| [...standardIds, sourceId, typeId], |
There was a problem hiding this comment.
Merge resolution (conflict). Base wrapped this createPromptsByIds call in headroom.retryOnQuota (#2874, the ~9s Semrush write-enforcement lag); WP-S2 added sourceId to the tag list. Combined both — the call stays inside retryOnQuota AND stamps source/semrush. Verified by markets-subworkspace.test.js (green).
| // ensureDimensionRoots adopts `source` as authorship and leaves the producing | ||
| // `source` key undefined. The server-owned resolve paths must fail LOUD rather | ||
| // than let an undefined root id degrade into a stranded root-level create. | ||
| describe('source root distinctness guard (mid-rename, WP-O6-gated)', () => { |
There was a problem hiding this comment.
Merge resolution (conflict). Base and WP-S2 each inserted a new describe block at this position — base's resolveClosedValueInjection (direct origin-dimension coverage) and WP-S2's source root distinctness guard (mid-rename 502 path). They cover different behaviour, so I kept BOTH rather than take one side. Both suites pass.
…LMO-6282) Introduce the 5th dimension root `source` (the producing system) as an OPEN, SERVER-OWNED dimension on the Semrush tag tree, and route the write-guard and resolve-or-create decisions through a new SERVER_OWNED_DIMENSIONS list so isClosedDimension() keeps only vocabulary validation. - prompt-tags.js: DIMENSION.SOURCE; SERVER_OWNED_DIMENSIONS + isServerOwnedDimension; canonicalizeSource() (§3.1 rule + guard); frozen exhaustive SOURCE_LABEL (CI-gated); PROXY_CREATE_SOURCE_VALUE / GENERATED_PROMPT_SOURCE_VALUE constants. - tag-tree.js: provision five roots with a distinctness guard so the producing `source` root is never conflated with the legacy authorship `source` root (WP-O6-gated); ensureClosedValue -> ensureServerOwnedValue. - handlers/tags.js: source create is resolve-or-create (no parentId, no enum). - handlers/prompts(.subworkspace).js: create-path injects source/config (constant); markets-subworkspace stamps source/semrush on generated prompts. - prompts-storage.js: mapRowToPrompt returns canonicalizeSource(row.source) (2nd derivation boundary; raw string on guard failure); source filter + sort key on lower(replace(source,'_','-')); updatePromptById never patches source. - controllers/brands.js: createPromptsByBrand ignores a body-supplied source. - docs: V2Prompt.source read-only (never on V2PromptInput); prompts-v2 source filter + sort enum; serenity create-tag type enum gains source. DRAFT — do not merge until WP-O6. Depends on the WP-S1 PE-client mock release for it-postgres; the SQL source filter/sort is unit-only until WP-S4's expression index. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…6282)
Defense-in-depth for the mid-rename window. ensureDimensionRoots deliberately
returns `source: undefined` while a project's `source` root still means authorship
(WP-O6-gated). That undefined id previously flowed through resolveClosedValueInjection
/ ensureServerOwnedValue into ensureChildren, where createProjectTags degrades to a
root-level create — silently minting a stranded `config`/`semrush` root tag.
Add requireServerOwnedRootId: the server-owned resolve paths now THROW a clear 502
("source dimension root not provisioned … WP-O6-gated") instead of proceeding with an
undefined parent. A no-op for the always-provisioned dimensions (origin/type/category).
Tests: tag-tree guard block (ensureDimensionRoots leaves source undefined;
ensureServerOwnedValue + resolveClosedValueInjection throw and issue no root-level
create) and a handler-level mid-rename create that fails cleanly (502, no
createPromptsByIds, no stranded root create).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…282) [WP-S2]
Stock PostgREST (the mysticat-data-service backend) cannot filter or order by
an inline SQL expression, so `.eq("lower(replace(source,'_','-'))", …)` and
`.order("lower(replace(source,'_','-'))")` returned a 400 at runtime on any
`?source=` filter or `sort=source`. The unit tests passed only because the mock
recorded the column string without validating it against PostgREST.
WP-S2 interim: match/sort on the raw `source` column. To preserve
"filter by the value the grid shows" across a producer's drift spellings, fold
the incoming filter value to canonical (via the shared `foldSourceValue`, now the
single definition of the transform) and match BOTH the hyphen and underscore
forms with `.in()`. Sort on the raw column (drift spellings may interleave — cosmetic).
The proper fix lands in WP-S4: a `source_canonical` generated column (+ index)
in mysticat-data-service, matched/ordered by column name.
- collapse the triple-copied fold into one exported helper (`foldSourceValue`)
- update unit + controller tests to assert raw-column `.in`/`.order`
- update the v2 OpenAPI source filter/sort descriptions
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ue (LLMO-6282) [WP-S2] The previous commit routed the v2 `source` list filter through the shared `foldSourceValue`, which called `value.trim()` directly — dropping the defensive `String()` the inline fold had. A non-string query param (parsed as a number or repeated-param array) would throw → 500. Restore the coercion inside the helper; `canonicalizeSource` type-guards before calling, so it is a no-op there. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
4868c8f to
d5e7fae
Compare
# [1.674.0](v1.673.3...v1.674.0) (2026-07-21) ### Bug Fixes * **deps:** trigger prod deploy for spacecat-shared-data-access 4.11.0 (LLMO-6401) ([#2879](#2879)) ([038e4da](038e4da)) ### Features * **serenity:** derive + inject prompt origin server-side — WP-O2b (LLMO-6275) ([#2866](#2866)) ([331071f](331071f)), closes [#2825](#2825) [#2867](#2867)
There was a problem hiding this comment.
Hey @aliciadriani,
Verdict: Approve - well-designed dimension refactor with thorough self-review; no blocking issues remain.
Complexity: HIGH - large diff (20 files); OpenAPI contract changes.
Changes: Introduces the 5th dimension root source (producing system) as an open, server-owned dimension with canonicalizeSource, SERVER_OWNED_DIMENSIONS refactor, and interim raw-column filter/sort (20 files).
Note: Recommend a human read before merge - this change modifies shared OpenAPI contracts (docs/openapi/*.yaml). The bot review is a complement to, not a replacement for, a human read here.
Note: CI checks are currently failing - it-postgres waits WP-S1; other failures are cascading dependencies. Resolve before merge.
Must fix before merge
No blocking issues found. All prior findings (PostgREST 400 on inline SQL expression, mid-rename requireServerOwnedRootId guard) have been addressed in subsequent commits.
Non-blocking (3): minor issues and suggestions
- nit:
ALL_DIMENSIONSis constructed as[...OPEN_DIMENSIONS, ...CLOSED_DIMENSIONS]which yields[category, source, intent, origin, type]- the ordering putssourcebefore the closed dimensions, which is fine but worth noting differs from theDIMENSION_ROOT_NAMESorder (category, intent, origin, type, source). Not a bug but a potential source of confusion if anyone assumes the two lists share ordering. -src/support/serenity/prompt-tags.js:180 - suggestion: The
sourceVariantsfilter generates[wanted, wanted.replace(/-/g, '_')]using aSetfor dedup, but for a value likegsc(no hyphens), both entries are identical and the Set deduplicates to one. Consider documenting that the single-element case is intentional (the test already covers it, so this is just a readability note). -src/support/prompts-storage.js:663 - nit: The
V2Prompt.sourceschema inschemas.yamlshowsdefault: "config"alongsidereadOnly: true- adefaulton a read-only field is technically a no-op in OpenAPI 3.0 (clients never send it, servers never need a default for output). Cosmetic only. -docs/openapi/schemas.yaml:6558
Previously flagged, now resolved
- PostgREST 400 on inline SQL expression for source filter/sort - now uses raw column with dual-spelling
.in()match - Mid-rename
sourceroot undefined flowing intoensureChildren- now guarded byrequireServerOwnedRootId(502) - Inline fold drift (third copy of the canonical transform) - extracted to shared
foldSourceValue - Non-string query param crash on
foldSourceValue-String()coercion restored
Note on SITES-47870 conflict (already identified in PR discussion): the source body-strip in brands.js removes a write surface that the SR "Track" flow relies on. This is already called out in the PR comments and gated by the DO-NOT-MERGE banner + WP-O6 prerequisite. Resolution should be settled before the gate lifts.
Skill: pr-review | Model: us.anthropic.claude-opus-4-6-v1[1m] | Duration: 4m 47s | Cost: $2.46 | Commit: d5e7fae35b19ec3a7a08af238ae05f0cdb35aadb
If this code review was useful, please react with 👍. Otherwise, react with 👎.
MysticatBot
added
ai-reviewed
|
Re-reviewed the full WP-S2 diff on the new base. My two prior fixes survived the rebase intact (
Requesting MysticatBot now that the base is |
aliciadriani
left a comment
aliciadriani
left a comment
There was a problem hiding this comment.
Material re-review at d5e7fae3 (base now main, rebased after O2b merged, MERGEABLE, 15411 unit). The sort/filter strategy changed since my last pass (a9539eec) — from the inline lower(replace(source,'_','-')) expression to a raw source column + foldSourceValue app-side fold. Assessed (a)/(b)/(c); /review-pr is clean. One CROSS-PR finding (report-only; S4 held) below.
(a) foldSourceValue — no drift. CONFIRMED. foldSourceValue(value) = String(value).trim().toLowerCase().replace(/_/g, '-') — the single definition of the transform, and canonicalizeSource now DELEGATES to it (then applies its empty/:/length/root-name guards). So the tag-write canonicalizer, the v2 list source filter, and the sort all fold through one function — consistent with the JS twin's prior trim→lower→_→- and the CLI Python twin (.strip().lower().replace('_','-')). No divergence.
(b) raw-column + app-fold — correct within the interim's stated scope; two documented residuals.
- FILTER: folds the incoming value (
wanted = foldSourceValue(source)) and matches.in('source', [wanted, wanted.replace(/-/g,'_')])on the raw column — so a filter forcitation-attemptfinds rows stored as eithercitation-attemptorcitation_attempt. It keys on the FOLDED SLUG (variant-expanded), never the display label. Residual (documented): the raw match is case-sensitive, so a mixed-case stored value (Citation_Attempt) is missed — the comment defers case-folding to WP-S4's canonical column. Acceptable givenprompts.sourceis machine-produced lowercase slugs; not a blocking bug. - SORT:
SORT_COLUMN_MAP.source = 'source'orders by the RAW column, so the two spellings of one producer interleave rather than sort together — the comment calls this cosmetic and defers folded ordering to WP-S4. So the sort does NOT key on the folded slug; it's a documented interim tradeoff driven by stock PostgREST 400ing on an inline expression order-by.
Both residuals are honest, commented, and WP-S4-deferred — no un-canonicalized boundary that returns wrong DATA, only imperfect ordering + a case-drift filter miss on unlikely inputs.
(c) core files + rebase conflicts — CONFIRMED, no drift. I hashed the four S2 core files (prompt-tags.js, tag-tree.js, prompts-storage.js, handlers/tags.js) at head vs the validated tip 4868c8f8: all four are BYTE-IDENTICAL (the commit-compare showing them differ was the three-dot rebase artifact, not reconstruction drift). Rebase conflict #1 (markets-subworkspace): the create is headroom.retryOnQuota(() => transport.createPromptsByIds(ws, pid, items, [...standardIds, sourceId, typeId]), {callSite:'createPromptsByIds'}) — the S4 sourceId (from ensureServerOwnedValue(..., GENERATED_PROMPT_SOURCE_VALUE)) is threaded into the tag_ids AND the write is wrapped in the LLMO-6190 retry; both concerns coexist correctly (this is S2, so createPromptsByIds is right — WP1 layers metadata separately). Rebase conflict #2 (tag-tree.test): both describe blocks are present (ensureServerOwnedValue / resolveClosedValueInjection / source root distinctness guard (mid-rename, WP-O6-gated) alongside the pre-existing resolveTypeValueInjection / findTagsInTree / …) — no clobber.
🚩 CROSS-PR CONSISTENCY FINDING (report-only — S4 #826 is held; do NOT fix here)
S2's current approach does NOT use S4's shipped index, and S2 anticipates a DIFFERENT S4 artifact than S4 actually delivered.
- S2 (this PR, now canonical) queries the RAW
sourcecolumn: filter.in('source', [hyphen, underscore]), sort.order('source'). Neither references the expressionlower(replace(source,'_','-')), so the Postgres planner will NOT use S4 #826'sidx_prompts_source_canonical ON public.prompts (lower(replace(source,'_','-')))— that expression index only serves a query whose predicate/order-by IS that expression, i.e. the ORIGINAL inline-SQL approach S2 abandoned (because stock PostgREST 400s on it). So relative to S2's current query shape, S4's index is an orphan. - The canonical FORM is consistent across the fleet:
foldSourceValue(S2) =lower(replace(source,'_','-'))(S4 index) =canonicalize_source(CLI) all produce the same slug (S4's index omits the trim, but stored slugs are untrimmed, so no practical divergence). It's the MECHANISM that diverges, not the value. - The mismatch to reconcile: S2's code comments (prompts-storage.js:434/522/659) say "WP-S4 adds a
source_canonicalGENERATED COLUMN (+ its index)" and that ordering/matching will move to that plain column — but S4 #826 shipped an EXPRESSION INDEX, no generated column. These are different artifacts: a generated column is PostgREST-nameable (.eq('source_canonical', …)/.order('source_canonical'), which is exactly what S2's raw+fold interim is a placeholder for); an expression index is only usable by the inline-SQL query that stock PostgREST cannot express. - Recommendation for the operator (before the 47 track merges): the raw-column-now →
source_canonicalgenerated-column-later path is the coherent, PostgREST-consumable target S2 already assumes; S4 #826's expression index does not serve it. Reconcile by having the WP-S4 DB change ship thesource_canonicalGENERATED COLUMN S2 expects (replacing / in addition to the expression index), OR consciously choose the inline-SQL+index path and give S2 a PostgREST-expressible route (view/RPC) — but that fights the 400 constraint that drove S2's pivot. Both #826 and #2867 are held pre-merge, so this is reconcilable now. Flagging, not fixing (S4 is held).
Verdict: /review-pr clean on the S2 code (a/b/c all sound, no reconstruction drift). The only open item is the cross-PR strategy/artifact reconciliation above. MysticatBot is re-requested for its first main-base pass (coordinator-triggered).
| * @param {unknown} value - a `source` value. | ||
| * @returns {string} the folded value. | ||
| */ | ||
| export function foldSourceValue(value) { |
There was a problem hiding this comment.
(a) CONFIRMED no drift: foldSourceValue = trim().toLowerCase().replace(/_/g,'-') is the SINGLE definition of the transform, and canonicalizeSource delegates to it. So the tag-write canonicalizer, the v2 list source filter, and the sort all fold identically — consistent with the CLI Python twin (.strip().lower().replace('_','-')) and the prior JS shape.
| // as `citation_attempt`. WP-S4's `source_canonical` generated column replaces | ||
| // this with a single canonical-column match (and adds case-folding, which raw | ||
| // matching drops). | ||
| const wanted = foldSourceValue(source); |
There was a problem hiding this comment.
(b) FILTER keys on the folded slug: wanted = foldSourceValue(source), matched via .in('source', [wanted, wanted.replace(/-/g,'_')]) — so citation-attempt finds citation_attempt rows too, on the raw column. Correct for the common _/- drift, and it's the folded slug not the display label. Documented residual: case-folding is dropped (raw match is case-sensitive), deferred to WP-S4's canonical column — acceptable for machine-slug data, not a blocking bug.
| // index) and ordering moves to that column name, so the two drift spellings order | ||
| // together; until then raw ordering only interleaves the `_`/`-` spellings of the | ||
| // same producer, which is cosmetic (source-dimension.md §3.1). | ||
| source: 'source', |
There was a problem hiding this comment.
(b) SORT orders by the RAW source column, so a producer's two drift spellings interleave rather than sort together — documented as cosmetic, folded ordering deferred to WP-S4. This is where the raw-column interim deviates from an ideal folded sort; driven by stock PostgREST 400ing on an inline-expression order-by. Not wrong data, just imperfect ordering until the canonical column exists.
| // FIX (WP-S2 interim): sort on the RAW `source` column. Stock PostgREST (the | ||
| // backend) cannot order by an inline SQL expression like | ||
| // `lower(replace(source,'_','-'))` — it returns 400 — and there is no canonical | ||
| // column to name yet. WP-S4 adds a `source_canonical` generated column (+ its |
There was a problem hiding this comment.
🚩 CROSS-PR FINDING (report-only, S4 held): this comment anticipates a WP-S4 source_canonical GENERATED COLUMN, but S4 #826 actually shipped an EXPRESSION INDEX idx_prompts_source_canonical ON (lower(replace(source,'_','-'))). S2's raw-column .in()/.order() do NOT use that expression index (the planner only picks it for the inline-SQL query PostgREST can't express — the approach S2 abandoned). The canonical VALUE is consistent (foldSourceValue == the index expr == the CLI), but the ARTIFACT S2 expects (a PostgREST-nameable generated column) is not what S4 delivered. Operator should reconcile before the 47 merges — the generated-column path is the coherent target; S4's expression index is currently an orphan relative to S2's queries. Do not fix here.
| // eslint-disable-next-line no-await-in-loop | ||
| await headroom.retryOnQuota( | ||
| () => transport.createPromptsByIds(workspaceId, projectId, items, [...standardIds, typeId]), | ||
| () => transport.createPromptsByIds( |
There was a problem hiding this comment.
(c) Rebase conflict #1 sound: the S4 sourceId (resolved via ensureServerOwnedValue(..., GENERATED_PROMPT_SOURCE_VALUE)) is threaded into the create's tag_ids ([...standardIds, sourceId, typeId]) AND the write is wrapped in the LLMO-6190 headroom.retryOnQuota. Both concerns coexist correctly. createPromptsByIds (not createPromptsWithMetadata) is right for S2's scope — WP1 #2877 layers the metadata create on top separately.
…6282) MysticatBot approved d5e7fae with no must/should fixes; clearing its 3 nits: - prompt-tags: document that ALL_DIMENSIONS is a membership set whose order intentionally differs from DIMENSION_ROOT_NAMES (provisioning order) - prompts-storage: note the single-element sourceVariants dedup (`gsc`) is intentional - schemas: drop `default: "config"` on the readOnly V2Prompt.source field (a default on a read-only output field is an OpenAPI 3.0 no-op) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
aliciadriani
left a comment
aliciadriani
left a comment
There was a problem hiding this comment.
Cleared MysticatBot's 3 non-blocking nits (commit 26f88ec). The bot's review of the prior commit had no must/should fixes — these are the optional polish items. Inline notes on each fix below.
| * this is grouped open-then-closed (`category, source, intent, origin, type`). | ||
| * Do not assume the two share an order. | ||
| */ | ||
| export const ALL_DIMENSIONS = Object.freeze([...OPEN_DIMENSIONS, ...CLOSED_DIMENSIONS]); |
There was a problem hiding this comment.
Fixed (bot nit). Documented that ALL_DIMENSIONS is a membership set whose grouped open-then-closed order (category, source, intent, origin, type) intentionally differs from DIMENSION_ROOT_NAMES' provisioning order (category, intent, origin, type, source) — so no one assumes the two share an order. No behaviour change (used only for .includes).
| const wanted = foldSourceValue(source); | ||
| // A value with no `_`/`-` (e.g. `gsc`) yields two identical variants that the | ||
| // Set collapses to one — a single-element `.in` is intentional, not a bug. | ||
| const sourceVariants = [...new Set([wanted, wanted.replace(/-/g, '_')])]; |
There was a problem hiding this comment.
Fixed (bot suggestion). Added a comment noting the single-element case is intentional: a value with no _/- (e.g. gsc) yields two identical variants that the Set collapses to one, so a one-element .in is expected, not a bug.
| no write surface — a create/update body cannot set it. Returned in its | ||
| CANONICAL form (`lower(replace(source, '_', '-'))`), so `agentic_traffic` | ||
| reads as `agentic-traffic`. | ||
| example: "config" |
There was a problem hiding this comment.
Fixed (bot nit). Dropped default: "config" from this readOnly V2Prompt.source field — a default on a read-only output field is an OpenAPI 3.0 no-op (clients never send it, servers never default output). example: "config" stays for docs.
…olumn (LLMO-6282) [WP-S2]
WP-S4 now ships `source_canonical` (GENERATED ALWAYS AS
lower(replace(source,'_','-')) STORED, btree-indexed), so PostgREST can name it
directly. Replace the raw-column + app-side variant-expansion interim:
- SORT_COLUMN_MAP.source -> 'source_canonical'; filter is a single
.eq('source_canonical', foldSourceValue(source)) — the incoming query-param
value is still folded (trim->lower->_->-) to align with the DB-canonicalized
column, but no more .in([hyphen, underscore]) variant expansion.
- The generated column canonicalizes at write time, so matching is now
case-insensitive and drift spellings sort together — the residuals the reviewer
noted (case-sensitivity, interleaved ordering) are gone.
- Dropped the "interim / WP-S4 adds it later" comments (it is real now) across
prompts-storage.js and the prompts-v2 OpenAPI filter/sort docs.
canonicalizeSource (tag-write path) and foldSourceValue (shared transform) are
unchanged; the rest of WP-S2 is unchanged.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
…e WP-S2 raw-column behavior (LLMO-6282) The OpenAPI descriptions drifted during the rebase to claim the `source` filter matches "on the canonical form ... case-insensitive" and sort orders drift spellings "together" — but the WP-S2 code does a raw-column dual-spelling `.in()` (sort on the raw column interleaves the `_`/`-` spellings; stored-side case is not folded). Correct both to describe the raw-column interim and point the full canonical + case fold at WP-S4's `source_canonical` column. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…match the WP-S2 raw-column behavior (LLMO-6282)" This reverts commit 3187325.
|
Correction to my earlier review summary (my prior comment predated The
The OpenAPI filter/sort descriptions already matched this; a doc commit I pushed briefly reverted them to the old wording by mistake — reverted in |
|
Picking up the SR "Track → Semrush" wiring (the layer that sits on top of the origin/source tracks), so flagging one more path for the same The Serenity create path loses
So whichever way the Not necessarily something to solve in this PR — just making sure the Serenity path is in scope when you + @dzehnder settle the |
3 participants
⛔ DO NOT MERGE YET — merge is gated on WP-O6 (LLMO-6280). O2a/O2b have landed in
main; WP-S2 waits WP-O6.(Serenity dimension-root program. See LLMO-6282 + serenity-fleet-handover/launch-checklist.md.)
WP-S2 — api-service:
SERVER_OWNED_DIMENSIONS+canonicalizeSourceIntroduces the 5th dimension root
source(the producing system) as an OPEN, SERVER-OWNED dimension on the Semrush tag tree, reusing the name the authorship root vacated in the 46 rename. Rebased ontomainnow that WP-O2b (makePromptTagInjector, tolerant tag-tree resolver, prompts-storage/brands changes) is merged.Jira: LLMO-6282
What changed (plan §WP-S2)
DIMENSION.SOURCE; appended toDIMENSION_ROOT_NAMES; joinsOPEN_DIMENSIONS(noCLOSED_DIMENSION_VALUESentry). NewSERVER_OWNED_DIMENSIONS = [intent, origin, type, source]+isServerOwnedDimension()carry the write-guard and resolve-or-create decisions;isClosedDimension()keeps only vocabulary validation.canonicalizeSource()(trim→lower→_→-,nullon empty/:/over-MAX_TAG_NAME_LEN/root-name).foldSourceValue()(the same fold, always returns a string) for filter/sort input. Frozen exhaustiveSOURCE_LABELwith a CI test that fails when a canonical value lacks a label.PROXY_CREATE_SOURCE_VALUE/GENERATED_PROMPT_SOURCE_VALUEconstants.sourceroot separate from a mid-rename authorshipsourceroot (map'ssourcekey isundefinedthere — the WP-O6 gate).ensureClosedValue→ensureServerOwnedValue.requireServerOwnedRootIdfails loud (502) rather than degrade an undefinedsourceroot into a stranded root-level create.sourcecreate is resolve-or-create (no parentId, no enum).source/config(constant, viamakePromptTagInjector's newsourceValue); update leaves source alone.generateAndAttachPromptsstampssource/semrush.mapRowToPromptreturnscanonicalizeSource(row.source)(raw string on guard failure).sourcefilter + sort key are on the rawsourcecolumn (interim): stock PostgREST 400s on an inlinelower(replace(source,'_','-'))expression, and there is no canonical column yet — WP-S4 adds asource_canonicalgenerated column and repoints filter/sort at it. Incoming filter value folded viafoldSourceValue.updatePromptByIdnever patches source.createPromptsByBrandignores a body-suppliedsource.V2Prompt.sourceread-only (never onV2PromptInput); prompts-v2sourcequery filter + sort enum; serenity create-tagtypeenum gainssource.Root assertions are membership, never a 5-root count.
Verify (node 24)
npm run type-check— passnpm run lint— pass (docs:lint /configurations/ambiguous-path warning is pre-existing)npm test(unit) — 15411 passing, coverage thresholds metNot in unit scope (by design)
it-postgreswaits the released WP-S1 PE-client mock image.sourcefilter/sort (folding drift spellings together) waits WP-S4'ssource_canonicalgenerated column; the raw-column filter/sort here is the interim.🤖 Generated with Claude Code