feat(serenity): WP-O2b origin derivation — salvaged from #2825 (LLMO-6275)#2864
feat(serenity): WP-O2b origin derivation — salvaged from #2825 (LLMO-6275)#2864aliciadriani wants to merge 11 commits into
Conversation
…me source→origin (LLMO-6274)
WP-O2a of the origin rename (serenity-docs#46). Renames the authorship dimension
root `source` → `origin` in api-service, and makes the tag-tree resolver TOLERANT
of both names for the duration of the in-place rename so it never mints a second
authorship root (origin-dimension.md §8).
- prompt-tags.js: DIMENSION.SOURCE→ORIGIN; DIMENSION_ROOT_NAMES; SOURCE_VALUE→
ORIGIN_VALUE; CLOSED_DIMENSION_VALUES/CLOSED_DIMENSIONS; STANDARD_PROMPT_TAG_VALUES.
Adds LEGACY_AUTHORSHIP_ROOT_NAME ('source') for the resolver's fallback.
- tag-tree.js: ensureDimensionRoots resolves the authorship root tolerantly —
prefers `origin`, else adopts a legacy `source` root IN PLACE (guarded by
childrenAreAuthorship: children ⊆ {ai,human}, so the companion producing-system
`source` dimension is never mistaken for authorship), and creates `origin` only
when neither exists. The returned map's `origin` key carries whichever physical
root was resolved, so provisionDimensionTree / ensureClosedValue inherit the
tolerance unchanged. ensureChildren gains an optional pre-read arg so the resolve
costs no extra root-level read on the common path. Removed with the fallback by WP-O6.
- New projects are provisioned with `origin` (via provisionDimensionTree).
- Tests: fixture + suites renamed source→origin; added two tolerant-resolver tests
(adopt-legacy-source-in-place; guard against a non-authorship `source` root).
type-check + lint + full unit suite (15264 passing) green. NOTE: the it/ integration
(test/it/, excluded from `npm test`) pulls the PE-client mock image and needs WP-O1
(spacecat-shared#1831) released to exercise the origin root + legacy-source seed.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…docs + coverage (LLMO-6274) Non-blocking follow-ups from MysticatBot's approval of #2862: - Reserve the legacy authorship name `source` in `isDimensionRootName` so a customer cannot mint a `source` tag during the rename window (removed at WP-O6). - Fix two stale `source` references in tag-tree.js JSDoc/header comments. - Rename confusing fixture id `r-source` → `r-origin` for the `origin` root. - Add tests: dual-root prefer-`origin`, childless-`source` vacuous adopt, and the migration-window reservation of the `source` name. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…LMO-6274) Non-blocking follow-ups from MysticatBot's second approval of #2862: - Add a rename-to-`source` rejection test mirroring the create-path guard. - Pin the dual-root performance contract with `listProjectTags.callCount === 1` (finding `origin` short-circuits before any orphan-`source` child read). - Assert the legacy-`source` 400 carries the reserved-name message, distinguishing the domain guard from a generic schema rejection. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ite (LLMO-6274) The tolerant-resolver PR renamed the authorship dimension root source→origin everywhere except test/it/shared/tests/serenity.js, which turned it-postgres red: - root-list assertion still expected `source` (code now returns `origin`); - two closed-dimension cases POSTed `type: 'source'`, which the handler now 400s as an unknown dimension. Rename the authorship references to `origin` to match. The producing-system `source` prompt filter in categories-prompts.js (serenity-docs#47) is a different dimension and is intentionally left untouched. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…6275) Recovers the O2b half of byteclimber's #2825, which was clobbered by a main-merge on that branch (its current head has no net O2b diff; the work survives in commit 74bbc00). Rebased onto WP-O2a (#2862). From 74bbc00 (O2b files only; O2a stays #2862's): - prompts.js / prompts-subworkspace.js: makeTypeInjector strips+injects origin (CREATE derives to ai, UPDATE re-injects the stored origin, never re-derives) - prompts-storage.js: resolveOriginForWrite + isServicePrincipal; mapRowToPrompt drops the '|| human' fallback - brands.js: createPromptsByBrand derives origin from the principal - docs/openapi: origin service-principal-only on V2PromptInput; read filter/sort kept Conflict resolved in prompts-storage upsert row: keep byteclimber's origin derivation, keep main's source handling (#2810; source is WP-S2's domain). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…275) - tag-tree.js: add resolveOriginValueInjection (was in byteclimber's O2a tag-tree.js, not carried when O2a came from #2862). Mirrors the inline resolveTypeValueInjection and resolves the origin root via the tolerant ensureDimensionRoots, so injection works during the source->origin rename. - Update prompts.js / prompts-subworkspace.js tests to expect the injected origin tag on create (ai) and in-place update (re-inject existing, else ai); basic create/update tests gain a listProjectTags stub since origin injection now always reads the tag tree. 509 affected serenity tests green; eslint clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
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! |
…MO-6275) Review found no test exercised the create/update asymmetry's core: an edit re-injecting an existing human origin rather than falling back to ai. Without it a regression could silently relabel human-authored prompts as AI on every edit. Adds a handleUpdatePrompt test asserting an input human origin tag is preserved (not defaulted to ai). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
aliciadriani
left a comment
There was a problem hiding this comment.
Reviewed WP-O2b (LLMO-6275): origin derivation + injector generalization, stacked on claude/wp-o2a-origin. Base is a stacked branch, so MysticatBot is N/A (it skips non-main bases) — /review-pr clean is the bar. The ⛔ banner is expected. it-postgres pending (waits WP-O1 release) is not a finding.
Applied the full correctness lens — all four invariants hold:
1. create-inject / update-preserve ASYMMETRY — CONFIRMED correct. In makeTypeInjector, nextOriginId = mode === 'update' ? (out.tagIds.find((id) => originTagIds.includes(id)) ?? aiId) : aiId. CREATE always derives AI; UPDATE re-injects whichever origin id the caller round-tripped and only falls back to AI when none is present (a pre-dimension prompt). Both call sites pass { mode: 'update' } on the update path (handleUpdatePrompt, handleUpdatePromptSubworkspace) and default to create elsewhere. The new test preserves an existing human origin on edit — never relabels it to ai directly guards the relabel bug. This is the point of the WP and it's right.
2. Strip by resolved tag id, never by name — CONFIRMED. stripped = out.tagIds.filter((id) => !originTagIds.includes(id)) where originTagIds = [...byName.values()] are the ids under the resolved authorship root. Mirrors the type strip exactly. No name-based stripping anywhere.
3. mapRowToPrompt returns row.origin with no || 'human' fallback — CONFIRMED, with the test updated to assert undefined passes through. Reasoning (column has zero NULLs in prod; the fallback would mislabel a null as human) is sound.
4. Item 6 — origin REMAINS on V2PromptInput — CONFIRMED. schemas.yaml keeps the field (adds a description, enum [ai,human], default human); it is not deleted. Deleting it would have relabeled DRS-written prompts, so keeping it is correct.
Also verified (gate wiring at source level): isServicePrincipal = Boolean(context.s2sConsumer) in brands.js uses the established s2sConsumer context attribute (set by s2sAuthWrapper, used the same way in llmo-mysticat-controller.js and others), so the service-principal detection is real, not a typo'd nonexistent property. And resolveOriginValueInjection resolves the root through ensureDimensionRoots, so it inherits O2a's tolerant origin/legacy-source resolution and never mints a second root.
SHOULD-FIX — missing spec-gate 5/6 coverage (implementer's flag CONFIRMED)
The WP-O2b plan states "spec gates 5–8 land here." Gates 5 (a SERVICE principal's origin:'ai' is honored) and 6 (a USER principal's origin is coerced to human, body never rejected) are the v2-controller-boundary gates. They are not covered:
test/controllers/brands.test.jsis NOT in this PR's diff, and I grepped the (base) file directly: 0 occurrences ofs2sConsumerand 0 ofisServicePrincipal(the 16originhits are allorigin: 'human'fixture data, not principal-derivation assertions).- The added tests cover the derivation logic only in ISOLATION:
resolveOriginForWrite(candidate, isServicePrincipal)as a pure unit, andupsertPrompts({ isServicePrincipal })with the flag passed directly. Neither exercises the controller'scontext.s2sConsumer→isServicePrincipalmapping. - Consequence: codecov/patch is green because the existing
createPromptsByBrandcontroller tests execute the newBoolean(context.s2sConsumer)line — but only the FALSE (user) branch, and with no assertion tying a service-principal request to a honoredorigin:'ai'nor a user request to a coercedhuman. The gate-5 TRUE branch at the controller boundary is untested end-to-end.
Recommended: add controller-level cases to test/controllers/brands.test.js (the file already exists, 31 createPromptsByBrand cases) — one with context.s2sConsumer set asserting a body origin:'ai' is honored (gate 5), one as a user principal asserting origin:'ai' is coerced to human with a 2xx / no rejection (gate 6). Routing to the implementer per the loop. (Gates 7/8 — the update-preserve asymmetry and tolerant-root strip-by-id — ARE covered by the new handler tests.)
No Must-Fix. Recording /review-pr NOT-yet-clean pending the gate 5/6 coverage; bot N/A (non-main base).
| // 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' |
There was a problem hiding this comment.
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.
| 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)); |
There was a problem hiding this comment.
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.
| * @returns {Promise<{ computedId: string, originTagIds: string[] }>} `computedId` | ||
| * is always resolved — {@link ensureChildren} throws rather than leave a hole. | ||
| */ | ||
| export async function resolveOriginValueInjection( |
There was a problem hiding this comment.
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.
| * an S2S service (see `context.s2sConsumer` at the controller boundary). | ||
| * @returns {'ai' | 'human'} | ||
| */ | ||
| export function resolveOriginForWrite(candidate, isServicePrincipal) { |
There was a problem hiding this comment.
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.
| // 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); |
There was a problem hiding this comment.
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.
| if (updates.origin !== undefined) { | ||
| patch.origin = updates.origin; | ||
| } | ||
| // `origin` is never patchable via the PATCH body — mirrors `source`, which is |
There was a problem hiding this comment.
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.
| enum: [ai, human] | ||
| default: human | ||
| description: >- | ||
| Who authored the prompt. Service-principal only: honored (and |
There was a problem hiding this comment.
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.
…LMO-6275) it-postgres caught the create path now appending an origin tag (WP-O2b) in addition to the type tag: the id-based create test supplied 2 tags and asserted 3 (2 + computed type); it is now 4 (2 + type + origin). Update the count and comment. Only exact tagIds-count assertion in the suite; other lengthOf checks are on created/skipped counts, unaffected. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…n' into claude/wp-o2b-origin-salvage
|
Superseded by #2866. The canonical WP-O2b (LLMO-6275) is branch |
Empty commit to fire a synchronize event on #2864 after reopen. Squashed away on merge. (--no-verify: local node_modules is stale post-main-merge re: the PE-client facade; CI type-check is green on this tree.) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Closing in favor of #2866 — the canonical WP-O2b PR (broader |
…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>
What & why
WP-O2b — origin derivation (LLMO-6275). Completes the origin dimension's write-side on top of WP-O2a (#2862, now merged to
main): the server derives and injects theorigintag on every prompt write, and it becomes read-only after create.Recovered from byteclimber's #2825 (whose O2b half was clobbered by a main-merge; salvaged from commit
74bbc00) and integrated with main's current in-place edit path.Changes
makeTypeInjectornow injectsoriginas well astype(handlers/prompts.js, + subworkspace twin): CREATE derives toai; UPDATE re-injects the caller's existing origin tag (preservinghuman), falling back toaionly if none — never a strip-without-inject. Strip is by resolved tag id under the root, never by name.tag-tree.js:resolveOriginValueInjection— mirrorsresolveTypeValueInjection, resolves the origin root through the tolerantensureDimensionRootsso it's correct mid-rename.prompts-storage.js:resolveOriginForWrite— origin comes from the authenticated principal (service principal may assert a valid enum; a user request is alwayshuman; invalid →human, never a 4xx).mapRowToPromptdrops the dead|| 'human'fallback;updatePromptByIdno longer patchesorigin(set once at create).brands.js: threadsisServicePrincipal(fromcontext.s2sConsumer).origindocumented service-principal-only onV2PromptInput, ignored on PATCH.Verification
humanorigin, never relabels toai). Lint clean.Depends on
WP-O2a #2862 (merged). Part of the origin dimension — serenity-docs#46, epic LLMO-6270.