feat(serenity): tolerant origin/source authorship-root resolver; rename source→origin (LLMO-6274)#2862
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>
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.
/review-pr — WP-O2a tolerant origin/source authorship-root resolver
Verdict: LGTM. The rename is mechanical and complete, and the tolerant resolver is carefully guarded — it never mints a second authorship root, keys the returned map on DIMENSION.ORIGIN regardless of the physical root, and adds no root-level read on the common path. Focus areas below.
Correctness (tolerant resolver)
- Never mints a second authorship root:
originis filtered out ofwantedwhenever an authorship root (eitheroriginor an adopted legacysource) was resolved, so the create call can only mintoriginon a genuinely fresh project. ✔ - Guard against the #47 companion
sourcedimension:childrenAreAuthorshiprestricts adoption to asourceroot whose children ⊆{ai, human}. A producing-systemsource(config,gsc, …) is correctly rejected andoriginis created instead. ✔ - Map-ordering contract: the result is rebuilt by iterating
DIMENSION_ROOT_NAMES, so key order is always[category, intent, origin, type]no matter which physical root backed authorship. ✔ (covered by the existing key-order assertion). ensureChildrenpre-read: thepreReadarg is used only for the optimistic first pass; both the create-race catch and the unechoed re-read still fetch a fresh level, so the race semantics are unchanged. ✔
One discussion point (see inline on childrenAreAuthorship)
The vacuous-true on a childless root means an empty source root is adopted as authorship. That is correct today (authorship source predates #47), but once the companion producing-system source dimension exists, a freshly-created-but-empty producing-system source on an origin-less project would be mis-adopted. Documented + removed at WP-O6, so I read it as an accepted, bounded risk rather than a blocker — flagging for the record.
Test coverage
The two new tests cover the adopt (legacy source + ai/human) and guard→create (non-authorship source) paths; the prefer-origin path is exercised by the existing "returns existing roots" test against the renamed fixture. Minor gap: no explicit test for the childless-source vacuous-adopt branch (see inline).
Out of scope per plan: test/it/ (needs WP-O1 released) and the pre-existing docs:lint /configurations/... warning.
(Posting as COMMENT — GitHub forbids the PR author self-approving.)
| CATEGORY: 'category', | ||
| INTENT: 'intent', | ||
| SOURCE: 'source', | ||
| ORIGIN: 'origin', |
There was a problem hiding this comment.
Authorship dimension root renamed source → origin. This is the canonical name the rest of the program (WP-O2a…O6) keys on; the legacy physical source root is tolerated at the resolver layer, not here.
| * the rename still carry a root named `source` (with `ai` / `human` beneath it); | ||
| * the tolerant resolver accepts it in place of `origin` until WP-O6 drops this. | ||
| */ | ||
| export const LEGACY_AUTHORSHIP_ROOT_NAME = 'source'; |
There was a problem hiding this comment.
New LEGACY_AUTHORSHIP_ROOT_NAME = 'source' — the single source of truth for the pre-rename name that tag-tree.js matches against. Good that it's a named export rather than a string literal buried in the resolver, and the doc explicitly scopes its lifetime to WP-O6.
| /** `source` values — who authored the prompt. */ | ||
| export const SOURCE_VALUE = Object.freeze({ | ||
| /** `origin` values — who authored the prompt. */ | ||
| export const ORIGIN_VALUE = Object.freeze({ |
There was a problem hiding this comment.
SOURCE_VALUE → ORIGIN_VALUE; values (ai/human) unchanged. The vocabulary is what childrenAreAuthorship diffs against, so keeping the values stable is what lets a legacy source root be recognized as authorship.
| */ | ||
| export const STANDARD_PROMPT_TAG_VALUES = Object.freeze([ | ||
| Object.freeze({ dimension: DIMENSION.SOURCE, name: SOURCE_VALUE.AI }), | ||
| Object.freeze({ dimension: DIMENSION.ORIGIN, name: ORIGIN_VALUE.AI }), |
There was a problem hiding this comment.
Standard AI-prompt seed now emits { dimension: origin, name: ai }. Callers resolve this pair against the project tree via ensureClosedValue, which inherits the resolver's tolerance — so on a legacy source project this still resolves to the physical source root's ai child.
| preRead, | ||
| ) { | ||
| const existing = await indexLevelByName(transport, semrushWorkspaceId, projectId, parentId, log); | ||
| const existing = preRead |
There was a problem hiding this comment.
preRead optional arg lets the tolerant resolver hand in the root-level index it already read, so the resolve costs no extra root read on the common path. Note it's used only for the optimistic first pass — the create-race catch (below) and the unechoed re-read both still fetch a fresh level, so race semantics are unchanged. One subtlety: existing is mutated in place with echoed nodes, so the caller's preRead map is mutated too; benign here because ensureDimensionRoots only reads from the returned byName afterward.
| // Tolerant authorship resolution: prefer `origin`; else adopt a legacy `source` root | ||
| // in place (guarded so the companion producing-system `source` dimension is never | ||
| // mistaken for authorship). | ||
| let authorshipId = existing.get(DIMENSION.ORIGIN); |
There was a problem hiding this comment.
Tolerant resolution order is right: prefer a real origin root, and only then consider adopting a legacy source. Because origin wins first, a partially-migrated project that has both roots resolves to origin and leaves the orphan source untouched (cleaned up at WP-O6) — no second root, no ambiguity.
| // Resolve-or-create every root except `origin`, and `origin` too UNLESS an authorship | ||
| // root was already found — creating it only then keeps the fresh-project path a single | ||
| // create call while never minting a second authorship root on a mid-rename project. | ||
| const wanted = DIMENSION_ROOT_NAMES.filter( |
There was a problem hiding this comment.
The wanted filter is what guarantees "never mint a second authorship root": origin is dropped from the create batch whenever authorshipId was resolved (via origin or adopted source), so a create can only produce origin on a genuinely fresh project. Fresh-project path stays a single batched create.
| for (const name of DIMENSION_ROOT_NAMES) { | ||
| roots.set( | ||
| name, | ||
| name === DIMENSION.ORIGIN ? (authorshipId ?? byName.get(name)) : byName.get(name), |
There was a problem hiding this comment.
Return-map assembly: iterating DIMENSION_ROOT_NAMES fixes key order to [category, intent, origin, type] regardless of physical root, and authorshipId ?? byName.get(name) puts the adopted/created id under the origin key. Correct for both branches — when authorshipId is set, origin was filtered from wanted so byName has no origin entry and the ?? falls through to the resolved id; when unset, byName.get('origin') holds the freshly-created id.
| expect(roots.get('type')).to.equal('created::type'); | ||
| }); | ||
|
|
||
| it('adopts a legacy `source` authorship root in place, minting no second `origin`', async () => { |
There was a problem hiding this comment.
Adopt path: legacy source root with ai/human children is adopted in place, roots.get('origin') maps to the physical source id, and createProjectTags is never called (no empty second root). Solid — this is the core migration-window behavior. Consider adding a sibling case with a childless source root to lock in the intentional vacuous-adopt branch.
| expect(transport.createProjectTags).to.not.have.been.called; | ||
| }); | ||
|
|
||
| it('does NOT adopt a `source` root carrying non-authorship values; creates `origin`', async () => { |
There was a problem hiding this comment.
Guard path: a source root carrying a non-authorship value (config) is correctly NOT adopted, and origin is created instead (created == ['origin']). This is the assertion that proves the #47 companion dimension is safe. Good complement to the adopt test.
There was a problem hiding this comment.
Hey @aliciadriani,
Verdict: Approve - well-guarded tolerant resolver with complete rename and solid test coverage.
Complexity: HIGH - medium diff; API surface signal (handlers).
Changes: Renames the authorship dimension root from source to origin and adds a tolerant resolver that adopts a legacy source root in place when it carries only authorship children (10 files).
Non-blocking (6): minor issues and suggestions
- nit: Stale JSDoc in
ensureClosedValuestill saysintent / source / type-src/support/serenity/tag-tree.js:542 - nit: Stale comment in file header still says "the
sourcevaluehuman" -src/support/serenity/tag-tree.js:29 - suggestion: Add
LEGACY_AUTHORSHIP_ROOT_NAMEto the reserved-name check until WP-O6 lands, so customers cannot create a tag namedsourceduring the migration window -src/support/serenity/prompt-tags.js - suggestion: Add a test for the dual-root scenario (project has both
originandsourceroots) to lock in the preference fororigin-test/support/serenity/tag-tree.test.js - suggestion: Add a test for the childless
sourcevacuous-adopt branch to document the intentional behavior before WP-O6 removes it -test/support/serenity/tag-tree.test.js - nit: Fixture id
r-sourcefor a root namedoriginis confusing; considerr-origin-test/support/serenity/handlers/tags.test.js:149
Skill: pr-review | Model: us.anthropic.claude-opus-4-6-v1[1m] | Duration: 2m 40s | Cost: $6.47 | Commit: 7bbc71d7f575b6926a4b8adceed8c9bb0120d054
If this code review was useful, please react with 👍. Otherwise, react with 👎.
MysticatBot
added
ai-reviewed
…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>
|
This PR will trigger a minor release when merged. |
aliciadriani
left a comment
aliciadriani
left a comment
There was a problem hiding this comment.
Addressed MysticatBot's non-blocking items — commit 9e0f3a9
Thanks @MysticatBot — all 6 items handled. Type-check + lint clean, serenity suite green (1005 passing; +4 new tests). Inline notes per change below.
| # | Item | Resolution |
|---|---|---|
| 1 | nit: stale JSDoc intent / source / type in ensureClosedValue |
Fixed → origin (tag-tree.js:542) |
| 2 | nit: stale header comment the \source` value `human`` |
Fixed → origin (tag-tree.js:29) |
| 3 | suggestion: reserve source name until WP-O6 |
isDimensionRootName now also rejects LEGACY_AUTHORSHIP_ROOT_NAME; covers both the create and rename call sites (prompt-tags.js:164) |
| 4 | suggestion: test dual-root prefer-origin |
Added (tag-tree.test.js:328) |
| 5 | suggestion: test childless-source vacuous adopt |
Added (tag-tree.test.js:352) |
| 6 | nit: confusing fixture id r-source for the origin root |
Renamed → r-origin (tags.test.js:149) |
Items 4 & 5 also close the two coverage gaps I flagged in my own pass, and item 3 hardens the vacuous-adopt discussion point by preventing a customer from introducing a fresh source tag during the window.
| */ | ||
| export function isDimensionRootName(name) { | ||
| return (/** @type {readonly string[]} */ (DIMENSION_ROOT_NAMES)).includes(name); | ||
| return name === LEGACY_AUTHORSHIP_ROOT_NAME |
There was a problem hiding this comment.
MysticatBot suggestion #3. isDimensionRootName now also reserves the legacy authorship name during the rename window, so a customer cannot mint a source tag that would collide with the legacy root the resolver still adopts. Extending the shared helper (rather than each call site) covers both handleCreateTag and handleUpdateTag at once. Scoped to WP-O6 in the JSDoc alongside the rest of the fallback.
| * unique per `(project, parent)`, not per project, so the resolve must be | ||
| * scoped to the parent — a bare-name lookup across the whole tree would | ||
| * conflate a sub-category `human` with the `source` value `human`. | ||
| * conflate a sub-category `human` with the `origin` value `human`. |
There was a problem hiding this comment.
MysticatBot nit #2. Stale file-header reference the \source` value `human`→origin`.
| * @param {string} semrushWorkspaceId | ||
| * @param {string} projectId | ||
| * @param {string} dimension - a closed dimension (`intent` / `source` / `type`). | ||
| * @param {string} dimension - a closed dimension (`intent` / `origin` / `type`). |
There was a problem hiding this comment.
MysticatBot nit #1. Stale @param dimension JSDoc intent / source / type → intent / origin / type.
| { id: 'r-category', name: 'category' }, | ||
| { id: 'r-intent', name: 'intent' }, | ||
| { id: 'r-source', name: 'source' }, | ||
| { id: 'r-origin', name: 'origin' }, |
There was a problem hiding this comment.
MysticatBot nit #6. Fixture id renamed r-source → r-origin to match the origin root it stubs — removes the source/origin naming mismatch.
| expect(transport.createProjectTags).to.not.have.been.called; | ||
| }); | ||
|
|
||
| it('400s the legacy `source` name too while the origin rename is in flight', async () => { |
There was a problem hiding this comment.
MysticatBot suggestion #3 (coverage). New test locks in the migration-window reservation: creating a tag named source now 400s, and upstream createProjectTags is never called.
| expect(roots.get('origin')).to.equal('made-origin'); | ||
| }); | ||
|
|
||
| it('prefers `origin` over a legacy `source` when a project carries BOTH roots', async () => { |
There was a problem hiding this comment.
MysticatBot suggestion #4. Dual-root scenario: a partially-migrated project carrying BOTH origin and legacy source resolves the origin key to the real origin root, leaves the orphan source untouched, and mints nothing. Locks in the prefer-origin precedence.
| expect(transport.createProjectTags).to.not.have.been.called; | ||
| }); | ||
|
|
||
| it('adopts a CHILDLESS legacy `source` root in place (vacuous authorship guard)', async () => { |
There was a problem hiding this comment.
MysticatBot suggestion #5. Childless-source vacuous-adopt branch: an empty legacy source root is still adopted in place (the intentional childrenAreAuthorship vacuous-true), no second origin minted. Documents the behavior explicitly before WP-O6 removes it.
There was a problem hiding this comment.
Hey @aliciadriani,
Verdict: Approve - all 6 prior items correctly addressed; incremental commit is clean.
Complexity: HIGH - large diff (905 lines); API surface signal.
Changes: Addresses all prior non-blocking review items - stale comments fixed, legacy source name reserved during migration, fixture renamed, and three new tests added for dual-root preference, childless adoption, and reservation enforcement (4 files).
Non-blocking (3): minor issues and suggestions
- suggestion: Add a test that renaming an existing tag TO
sourceis also rejected during the migration window (mirrors the new create-path test) -test/support/serenity/handlers/tags.test.js - suggestion: Assert
listProjectTags.callCount === 1in the dual-root test to pin the performance contract (no child-level read for the orphansource) -test/support/serenity/tag-tree.test.js:753 - suggestion: Assert the error message matches
/reserved/or/dimension root/in the legacy-source-400 test to distinguish the domain guard from a generic schema validation rejection -test/support/serenity/handlers/tags.test.js:495
Previously flagged, now resolved
- Stale JSDoc
intent / source / typeinensureClosedValuefixed - Stale header comment
the source value humanfixed isDimensionRootNamenow rejects legacysourceduring migration window- Dual-root prefer-origin test added
- Childless-source vacuous-adopt test added
- Fixture id
r-sourcerenamed tor-origin
Skill: pr-review | Model: us.anthropic.claude-opus-4-6-v1[1m] | Duration: 1m 15s | Cost: $5.07 | Commit: 9e0f3a950ca28f36c03e98e4bf5b2d8c5a062bdf
If this code review was useful, please react with 👍. Otherwise, react with 👎.
…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>
aliciadriani
left a comment
aliciadriani
left a comment
There was a problem hiding this comment.
Addressed MysticatBot round-2 suggestions — commit 81ca372
Thanks @MysticatBot — all 3 non-blocking test-hardening suggestions handled. Type-check + lint clean, affected suites green (117 passing across tag-tree + tags). Inline notes per change below.
| # | Suggestion | Resolution |
|---|---|---|
| 1 | rename-to-source also rejected |
Added a handleUpdateTag test mirroring the create-path guard (tags.test.js:918) |
| 2 | pin dual-root perf contract | Assert listProjectTags.callCount === 1 — no orphan-source child read (tag-tree.test.js:352) |
| 3 | assert domain guard message | Both legacy-source 400 tests now assert /reserved dimension root name/ (tags.test.js:264, :933) |
All test-only; no production changes this round.
| expect(transport.updateProjectTag).to.not.have.been.called; | ||
| }); | ||
|
|
||
| it('400s on a rename to the legacy `source` name during the migration window', async () => { |
There was a problem hiding this comment.
MysticatBot suggestion #1. Rename-path guard: mirrors the create-path test — handleUpdateTag renaming a tag onto source 400s with the reserved-name message and never calls updateProjectTag. Both write paths share the isDimensionRootName guard, so both are now pinned.
| expect(transport.createProjectTags).to.not.have.been.called; | ||
| // Performance contract: finding `origin` short-circuits before any legacy | ||
| // adoption, so no child-level read of the orphan `source` — exactly ONE read. | ||
| expect(transport.listProjectTags).to.have.callCount(1); |
There was a problem hiding this comment.
MysticatBot suggestion #2. Performance contract pinned: with both roots present, finding origin short-circuits before any legacy adoption, so childrenAreAuthorship never runs and the orphan source is never child-read — exactly ONE listProjectTags call.
| )).to.be.rejected.then((err) => { | ||
| expect(err.status).to.equal(400); | ||
| // The domain guard, not a generic schema rejection. | ||
| expect(err.message).to.match(/reserved dimension root name/); |
There was a problem hiding this comment.
MysticatBot suggestion #3. The legacy-source create 400 now asserts /reserved dimension root name/, distinguishing the domain guard from a generic schema-validation rejection. The rename test (:933) asserts the same.
There was a problem hiding this comment.
Hey @aliciadriani,
Verdict: Approve - all 3 prior suggestions correctly addressed; test-only commit is clean.
Complexity: HIGH - large diff (939 lines); API surface signal.
Changes: Addresses all prior non-blocking review items - rename-to-source rejection test, dual-root performance contract assertion, and domain-guard error message assertions (2 files).
Previously flagged, now resolved
- Rename-to-
sourcerejection test added (mirrors the create-path guard on the update path) listProjectTags.callCount === 1assertion pins the dual-root short-circuit performance contract- Error message regex
/reserved dimension root name/distinguishes the domain guard from generic schema validation on both create and rename paths
Skill: pr-review | Model: us.anthropic.claude-opus-4-6-v1[1m] | Duration: 0m 40s | Cost: $3.56 | Commit: 81ca3728f1f99907af407db6786041e0cade55a4
If this code review was useful, please react with 👍. Otherwise, react with 👎.
aliciadriani
left a comment
aliciadriani
left a comment
There was a problem hiding this comment.
Reviewed WP-O2a: the tolerant origin/source authorship-root resolver plus the mechanical source→origin rename across prompt-tags.js / tag-tree.js / handler doc comments.
Rename correctness: verified no stray SOURCE_VALUE / DIMENSION.SOURCE / SOURCE_ROOT_TAG_ID references remain anywhere in src/ or test/ outside the diff — the only surviving literal 'source' is the intentional LEGACY_AUTHORSHIP_ROOT_NAME constant. Clean.
Resolver logic (ensureDimensionRoots / childrenAreAuthorship in tag-tree.js): traced all four branches by hand against the new tests —
originexists → used directly, legacysourcenever even inspected (no wasted read). Matches "prefers origin over legacy source when project carries BOTH" test.- No
origin,sourceexists with children ⊆ {ai,human} → adopted in place,originnever created. Matches "adopts a legacy source root" test. - No
origin,sourceexists with non-authorship children (e.g.config) → guard correctly rejects,origincreated fresh. Matches "does NOT adopt" test. - No
origin,sourceexists but childless → vacuous pass, adopted. Matches "adopts a CHILDLESS legacy source root" test — this vacuous-true branch is intentional per the doc comment and I agree with the reasoning (an unpopulated root can't yet disagree with authorship). - Neither exists (fresh project) → falls through to the pre-existing create-all-four path, unchanged.
All consistent with the id-stable / membership-check correctness lens (no count or set-equality assertions snuck in) and the in-place-adoption contract (never a second authorship root minted). This is well-designed and well-tested.
Only gap I see: no test exercises the existing/preRead plumbing itself for a call-count assertion (i.e., proving ensureChildren truly skips the extra indexLevelByName read on the common no-legacy-root path) — the tests assert behavior/outcomes, not read-count, so the "no extra read on the common path" claim in the docstring is unverified by the test suite itself. Minor, not blocking — the logic is correct by inspection either way.
CI note: it-postgres was still pending/in-progress on both matrix jobs as of my review (not yet fully green), though everything else (build, type-check, lint, codecov) had passed. Per the correctness lens, absent test/it/ runs aren't a scope issue for this draft PR, but flagging the still-pending checks for visibility.
No Must-Fix findings. Requesting MysticatBot below since base is main.
| * @param {object} [log] | ||
| * @returns {Promise<boolean>} | ||
| */ | ||
| async function childrenAreAuthorship(transport, semrushWorkspaceId, projectId, rootId, log) { |
There was a problem hiding this comment.
childrenAreAuthorship: the vacuous-true case (empty children set) is a deliberate design choice per the docstring — an unpopulated root can't yet disagree with authorship, so it's adopted rather than shadowed by a fresh origin. Confirmed this is exercised by the "CHILDLESS legacy source root" test. Agreed this is correct: a blind reject here would force a spurious second root the first time a project's source root simply hasn't been populated yet.
| * @returns {Promise<Map<string, string>>} root name → tag id, in root order, with the | ||
| * `origin` key carrying the resolved authorship root's id. | ||
| */ | ||
| export async function ensureDimensionRoots(transport, semrushWorkspaceId, projectId, log) { |
There was a problem hiding this comment.
ensureDimensionRoots: traced the full branch matrix (origin-exists / legacy-adopt / legacy-reject-mint-origin / childless-legacy-adopt / neither-exists) against the four new tests plus the two pre-existing ones — all consistent, no case where a second authorship root could be minted alongside an already-resolved one. The wanted filter (name !== DIMENSION.ORIGIN || !authorshipId) correctly excludes origin from the create pass exactly when an authorship root (real or legacy) was already found, and the final roots map always keys the authorship dimension under DIMENSION.ORIGIN regardless of which physical root backs it — so callers (provisionDimensionTree, ensureClosedValue) get the tolerance for free, verified in the diff below.
| parentId, | ||
| wanted, | ||
| log, | ||
| preRead, |
There was a problem hiding this comment.
New optional preRead param on ensureChildren, reused by ensureDimensionRoots to avoid a second root-level read after the tolerant resolve already did one. Correct as implemented (existing = preRead ?? await indexLevelByName(...)); the only nit is there's no test asserting the read isn't duplicated (an interaction/call-count test), so this optimization's correctness rests on code inspection rather than a regression-guarding test. Not blocking.
| * @param {string} name - a bare tag name. | ||
| * @returns {boolean} | ||
| */ | ||
| export function isDimensionRootName(name) { |
There was a problem hiding this comment.
isDimensionRootName now also reserves the legacy source name (not just the four current roots), which is the right call — it prevents a customer from minting a category/tag named source that could collide with (or be confused for) the legacy authorship root the resolver still adopts. Verified this isn't a new restriction in practice: source was already reserved pre-rename as the (then-current) authorship root name, so no existing customer-tag-creation behavior regresses here — it's continuity, not a new restriction, until WP-O6 drops it.
…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>
aliciadriani
left a comment
aliciadriani
left a comment
There was a problem hiding this comment.
/review-pr re-pass + CI-red fix — commit 97466e7
Review re-pass
Re-read the full PR diff. The serenity source + unit changes are unchanged and remain clean — no new must-fix / should-fix / nits beyond what MysticatBot and I already closed across the three prior rounds. One real finding surfaced by CI, addressed below.
Why CI was red — ci / it-postgres (3 failing)
All three failures were in test/it/shared/tests/serenity.js, the one file the source → origin rename had not reached (it was deferred as "integration, needs WP-O1"). Since the branch's base main now resolves origin, the stale integration assertions broke:
| Test | Symptom | Cause |
|---|---|---|
root-list (:323) |
expected [origin] members [source] |
assertion still expected the old root name |
idempotent closed value (:483) |
expected 400 to equal 200 |
POSTed type: 'source' — now an unknown dimension → 400 |
closed-value rename 400 (:505) |
expected 400 to equal 200 |
same type: 'source' setup step 400s before the PATCH |
Fix: renamed the authorship references to origin in that suite — the identical mechanical rename already applied everywhere else. The producing-system source prompt filter in categories-prompts.js (serenity-docs#47) is a different dimension and is intentionally left untouched. Lint clean; the it-postgres job is re-running now to confirm green.
This closes the loop's CI gate: the integration failure was a real incomplete-rename bug, not the previously-assumed missing mock seed.
| expect(roots.status).to.equal(200); | ||
| expect(roots.body.items.map((t) => t.name)) | ||
| .to.have.members(['category', 'intent', 'source', 'type']); | ||
| .to.have.members(['category', 'intent', 'origin', 'type']); |
There was a problem hiding this comment.
CI fix (it-postgres failure #1). Root-list assertion renamed source → origin. The list endpoint returns the physical root names, and post-rename the resolver provisions/adopts an origin root — the expected [origin] to have members [source] mismatch was purely this stale expectation.
| }); | ||
|
|
||
| it('POST /serenity/tags resolves a closed-dimension tag idempotently (source/intent/type)', async () => { | ||
| it('POST /serenity/tags resolves a closed-dimension tag idempotently (origin/intent/type)', async () => { |
There was a problem hiding this comment.
CI fix (it-postgres failure #2). This case POSTed type: 'source', which parseCreateTagBody now rejects (400) since source is no longer in ALL_DIMENSIONS. Renamed the dimension to origin in the title and all three request bodies below — the closed value resolves against the authorship root (whether the mock seeds a real origin or a legacy source the resolver adopts), so it now returns 200 idempotently.
| expect(first.body).to.include({ type: 'origin', name: 'ai' }); | ||
| expect(first.body.id).to.be.a('string').that.is.not.empty; | ||
| // The value hangs under the `source` root, never at the root level. | ||
| // The value hangs under the `origin` root, never at the root level. |
There was a problem hiding this comment.
Comment updated to origin root for consistency. Note: the separate source prompt filter in categories-prompts.js (source: 'gsc' | 'config') is the producing-system dimension from serenity-docs#47 — a different source, correctly NOT renamed by this PR.
There was a problem hiding this comment.
Hey @aliciadriani,
Verdict: Approve - clean mechanical rename completing the integration test alignment; no findings.
Complexity: HIGH - large diff (987 lines); API surface signal.
Changes: Completes the source→origin rename in the integration test suite (test/it/shared/tests/serenity.js), fixing the CI-red it-postgres failures caused by stale assertions against the renamed dimension root (11 files total across the PR).
Previously flagged, now resolved
- Rename-to-
sourcerejection test added (mirrors the create-path guard on the update path) listProjectTags.callCount === 1assertion pins the dual-root short-circuit performance contract- Error message regex
/reserved dimension root name/distinguishes the domain guard from generic schema validation on both create and rename paths
Skill: pr-review | Model: us.anthropic.claude-opus-4-6-v1[1m] | Duration: 0m 54s | Cost: $2.68 | Commit: 97466e7153de07d3ebca2f064ab5138dec42b148
If this code review was useful, please react with 👍. Otherwise, react with 👎.
GitHub Actions
Inactive
There was a problem hiding this comment.
Review— fresh re-pass (commit 97466e7)
Summary
WP-O2a renames the authorship dimension root source → origin and makes ensureDimensionRoots resolve it tolerantly during the in-flight migration: prefer an origin root, else adopt a legacy source root in place (guarded so the companion producing-system source dimension is never mistaken for authorship), and create origin only when neither exists. The returned map always keys on origin, so provisionDimensionTree / ensureClosedValue inherit the tolerance transparently. Scope is 11 files (2 production-logic, 2 comment-only, the rest tests + fixture). Quality is high — the production logic is confined to tag-tree.js / prompt-tags.js, thoroughly commented with the WP-O6 removal path, and the diff is dominated by a clean mechanical rename. This pass surfaces no new findings.
Must Fix (blocks merge)
None. No correctness, security, or data-loss issues. Verified this pass:
- The resolver cannot mint a second authorship root —
originis filtered out of the create batch whenever an authorship root was resolved. - The fails-closed invariant holds: the rebuilt roots map always carries all four keys, and
originresolves toauthorshipId ?? byName.get('origin'), provably defined in every branch. childrenAreAuthorshipsees the full child level (indexLevelByName→listProjectTagTreepages internally), so the{ai, human}guard has no pagination blind spot.- The pre-read map mutation in
ensureChildrenis benign —ensureDimensionRootsonly reads from the returnedbyNameafterward.
Should Fix
None. Naming (authorshipId, childrenAreAuthorship, LEGACY_AUTHORSHIP_ROOT_NAME) is clear; test coverage spans prefer / adopt / childless-vacuous-adopt / guard→create / dual-root paths plus the migration-window name reservation on both create and rename paths.
Nits (optional)
None outstanding — the earlier nits (two stale comments, the r-source fixture, the reserved-name message assertion, the dual-root callCount) were all resolved across the prior rounds.
What's Good
- The
childrenAreAuthorshipguard is the elegant crux: it letssourceandorigincoexist safely, with the vacuous-true on a childless root now backstopped by reserving thesourcename so a customer can't introduce an ambiguous emptysourcemid-migration. - No extra reads on the common path — the pre-read arg threads the already-fetched root level into
ensureChildren; only a present legacysourcetriggers the one extra child read. - Disciplined migration hygiene — every tolerant branch is tagged for removal at WP-O6, and the producing-system
sourcedimension (serenity-docs#47) is explicitly carved out and left untouched (including incategories-prompts.js). - CI is fully green, including the
it-postgresintegration suite that this PR's final commit fixed.
Bottom line: clean across all six review phases — no must-fix, should-fix, or nits. Both /review-pr and MysticatBot have converged with no outstanding suggestions, and CI is green.
aliciadriani
changed the title
…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>
|
📎 Stacked PR: #2866 (WP-O2b — origin derivation + injector generalization, LLMO-6275) is stacked on this branch ( |
…e (LLMO-6275) WP-O2b of the origin rename (serenity-docs#46), stacked on WP-O2a (claude/wp-o2a-origin, #2862). Generalizes the prompt-tag injector and the v2 prompts write surface so `origin` is derived from the write path, never asserted by a user, with the create/update asymmetry origin-dimension.md §3 requires. Serenity tag path (handlers/prompts.js + prompts-subworkspace.js): - makeTypeInjector -> makePromptTagInjector: on CREATE strips any caller-supplied tag id beneath the origin root (by resolved id, never by name) and injects the derived `human`; on UPDATE leaves origin untouched -- the stored value the caller echoes rides through, never re-derived (gates 7, 8; create arm of 5). - resolveClosedValueInjection generalizes resolveTypeValueInjection. v2 prompts API (controllers/brands.js + prompts-storage.js): - createPromptsByBrand derives origin from the principal: IMS/JWT user -> `human` (body ignored, never rejected); service principal (admin x-api-key, e.g. DRS) -> validated body value (gates 5, 6 -- the DRS origin:'ai' contract). - upsertPrompts preserves the stored origin on a match-update; updatePromptById no longer patches origin; mapRowToPrompt returns row.origin verbatim. - OpenAPI: origin documented as service-principal-only / read-only for users and not patchable on update; the origin query filter + sort enum stay. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
aliciadriani
changed the title
GitHub Actions
Inactive
…e (LLMO-6275) WP-O2b of the origin rename (serenity-docs#46), stacked on WP-O2a (claude/wp-o2a-origin, #2862). Generalizes the prompt-tag injector and the v2 prompts write surface so `origin` is derived from the write path, never asserted by a user, with the create/update asymmetry origin-dimension.md §3 requires. Serenity tag path (handlers/prompts.js + prompts-subworkspace.js): - makeTypeInjector -> makePromptTagInjector: on CREATE strips any caller-supplied tag id beneath the origin root (by resolved id, never by name) and injects the derived `human`; on UPDATE leaves origin untouched -- the stored value the caller echoes rides through, never re-derived (gates 7, 8; create arm of 5). - resolveClosedValueInjection generalizes resolveTypeValueInjection. v2 prompts API (controllers/brands.js + prompts-storage.js): - createPromptsByBrand derives origin from the principal: IMS/JWT user -> `human` (body ignored, never rejected); service principal (admin x-api-key, e.g. DRS) -> validated body value (gates 5, 6 -- the DRS origin:'ai' contract). - upsertPrompts preserves the stored origin on a match-update; updatePromptById no longer patches origin; mapRowToPrompt returns row.origin verbatim. - OpenAPI: origin documented as service-principal-only / read-only for users and not patchable on update; the origin query filter + sort enum stay. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
# [1.671.0](v1.670.0...v1.671.0) (2026-07-20) ### Features * **serenity:** tolerant origin/source authorship-root resolver; rename source→origin (LLMO-6274) ([#2862](#2862)) ([cad46af](cad46af))
|
🎉 This PR is included in version 1.671.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
Post-merge it-postgres (gate-3) validation — cannot run against
|
✅ it-postgres gate-3 now GREEN — via the deferred bump (#2869)Follow-up to my earlier "cannot run yet" note. The blocker was the deferred PE-client bump; I landed it in #2869 ( Result — Serenity resolver assertions that now pass e2e against the 1.15.0 Scope note (accuracy): the IT compose runs the PE mock with Bottom line: WP-O2a's tolerant resolver is now validated post-merge at the it-postgres layer (once #2869 lands to move |
## What Bumps `@adobe/spacecat-shared-project-engine-client` **1.14.0 → 1.15.0** (package.json + lockfile only). ## Why This is the dependency bump #2862 (WP-O2a) **explicitly deferred** — *"the PE-client dependency bump to the WP-O1 release is deferred until that release exists."* WP-O1 is now released (1.15.0), so this lands it. It unblocks the **it-postgres serenity gate-3** validation of the tolerant `origin`/`source` authorship-root resolver. The IT harness pins the Semrush mock image tag to the **installed** client version (`test/it/postgres/setup.js` → `SERENITY_PE_MOCK_TAG`, exact match, no fallback). The `origin` root rename + `legacy-source-workspace` seed that the gate-3 cases run against shipped only in **1.15.0** (WP-O1, spacecat-shared `3ead32b` / LLMO-6273). While main pinned 1.14.0, the harness pulled the pre-rename mock and gate-3 could not be exercised (the renamed `['category','intent','origin','type']` assertion would fail; no legacy fixture to adopt). With this bump the harness derives `SERENITY_PE_MOCK_TAG=1.15.0`, so the CI `it-postgres` job now exercises: - existing `origin` root resolves; - legacy `source` root (children ⊆ `{ai,human}`) **adopted in place**, no 2nd root minted; - fresh project mints `origin`; - membership (not set-equality) assertions in `childrenAreAuthorship`. ## Risk **Mock-only release.** 1.15.0 over 1.14.0 touched only the package's `mock/` dir — the client runtime and typings are unchanged, so there is no production behavior change (no coordinated merge window needed, unlike the rename in #2862). Lockfile delta is the single package entry. ## Verification - `npm run type-check` ✅ (clean) - Lockfile + `node_modules` resolve to 1.15.0 ✅ - Real gate-3 e2e validation runs here in CI (`it-postgres` has ECR access for the private data-service image). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Alicia Adriani <aadriani+adobe@adobe.com> Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
…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>
3 participants
WP-O2a — tolerant
origin/sourceauthorship-root resolver + renameSecond code work-package of the origin rename (serenity-docs#46), after WP-O0 (LLMO-6272) recorded PASS / id-stable. Tracks LLMO-6274.
Changes (
src/support/serenity)prompt-tags.js— authorship rootsource→origin:DIMENSION.SOURCE→ORIGIN,DIMENSION_ROOT_NAMES,SOURCE_VALUE→ORIGIN_VALUE,CLOSED_DIMENSION_VALUES/CLOSED_DIMENSIONS,STANDARD_PROMPT_TAG_VALUES. AddsLEGACY_AUTHORSHIP_ROOT_NAME('source') for the resolver fallback.tag-tree.js— the tolerant resolver (the point of this WP).ensureDimensionRootsprefers anoriginroot, else adopts a legacysourceroot in place — guarded bychildrenAreAuthorship(children ⊆{ai,human}, so the companion producing-systemsourcedimension is never mistaken for authorship) — and createsoriginonly when neither exists. A blind create would mint an empty second authorship root the moment code and data disagree (spec §8). The returned map'soriginkey carries whichever physical root was resolved, soprovisionDimensionTree/ensureClosedValue(and new-project provisioning) inherit the tolerance unchanged.ensureChildrengains an optional pre-read arg so the tolerant resolve adds no extra root-level read on the common path. All of this is removed with the fallback by WP-O6.handlers/tags.js,handlers/markets-subworkspace.js.Tests
source→origin; added two tolerant-resolver tests: adopt-legacy-source-in-place (no second root created), and the guard that asourceroot carrying non-authorship values (config) is not adopted (origincreated instead) — spec §7 gate 3.type-check✅ ·lint✅ · full unit suite 15264 passing, coverage thresholds met ✅ (node 24, as api-service requires).Not in this PR / blocked
test/it/integration (excluded fromnpm test) pulls the PE-client mock image and needs WP-O1 (spacecat-shared#1831) released to exercise theoriginroot +legacy-source-workspaceseed. Also, the PE-client dependency bump to the WP-O1 release is deferred until that release exists (noSEED_IDSare pinned in api-service, so nothing to move — the harness resolves ids at runtime).Notes
docs:lintpre-commit warning about ambiguous/configurations/...paths is pre-existing indocs/openapi/api.yaml(untouched here).🤖 Generated with Claude Code