fix(serenity): read and write the intent root under one name | LLMO-6986 - #3054
Conversation
The intent dimension root is `$abv_tags$intent` upstream. Resolve it by that name alone: five Elements calls per enrichment request, one prefix in the filter-dimension reader, and one name in the tag-tree resolver. LLMO-6986 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ent-root-tolerance
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
Validated on the shared Target: brand
So the runtime behaviour matches what the tests pin, including the failure mode on an un-swept project — which is the reason this stays a draft. A second point falls out of the run: dev itself has an un-swept project, so whatever sweep satisfies acceptance criterion 1 should cover dev, not only the customer fleet. Not covered: Lambda log inspection for this window. The dev backend sourcetype returned nothing in Splunk for the run window and the CloudWatch session had expired, so "no new errors" rests on the HTTP responses rather than on logs. |
Removing the tolerance took its documentation, its detector and its tests with it, leaving a project the rename missed to split the intent dimension in silence. - ensureDimensionRoots warns when it mints `$abv_tags$intent` beside a pre-rename `intent` root, mirroring the guardrail the `source` to `origin` rename already carries in the same function. `byName` already indexes the level, so this costs no extra read. - Its doc block described the deleted alias as still protecting against that split; it now states what the resolver actually does to both renamed roots. - `intent` is reserved on the read surface, so a pre-rename tag lands in the generic `tags` array instead of a customer-facing dynamic filter group. - The catch-all drops anything carrying `$abv_tags$`, so the hiding control tracks the marker rather than a list of known prefixes. - The OpenAPI contract no longer tells consumers to match both spellings. - Fixtures naming the pre-rename root are renamed, and the create batch is pinned: one of them made the suite exercise the split path and pass. - The enrichment-failure warn carries an event key and is asserted against a real logger; a blank userIntent cannot otherwise be told from a failed call. LLMO-6986 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
This PR will trigger a patch release when merged. |
rainer-friederich
left a comment
There was a problem hiding this comment.
Hey @rainer-friederich,
⚠ Degraded review - no spec document was found for this change (searched the PR links, the touched repos' docs, the architecture/guidelines docs, and linked Jira). This review covers code-level quality but could not validate the change against an agreed design, so confidence is reduced. Add a spec link (PR template section 4) and re-request review for a full-confidence pass.
Verdict: Request changes - now addressed in 62729160a, which is pushed. The findings and their resolutions are recorded below.
Changes: removes the dual-spelling tolerance for the Semrush AIO intent dimension root so it is read and written only as $abv_tags$intent (10 files, plus 12 touched by the follow-up).
Posted as a comment rather than a formal Request-changes because GitHub does not accept a non-comment review state from the PR author.
Must fix before merge
These were not eight independent problems. They were one: the tolerance, the paragraph explaining why it existed, the detector that would catch its absence, and the tests that pinned it were all deleted together, so nothing was left to notice an un-swept project. All eight are fixed in 62729160a.
The un-swept state was silent and mis-documented:
- [Critical]
ensureDimensionRoots' JSDoc still stated that the pre-rename name "is passed as an ALIAS" and that a project the rename has not reached "keeps its populated root and gets no second one" - on the exact function that, after this PR, mints a second root and strands every prompt tagged under the old one. An affirmative false safety claim, and the only in-repo record of why the tolerance existed. Rewritten to state what the resolver does to both renamed roots and to name the consequence of a gate bypass. - [Important] Nothing detected the split-root state, while the sibling
sourcetooriginrename in the same function deliberately keeps a warn for the identical shape. Added the intent counterpart. It needs no extra read -ensureChildrenalready returns the level index - so it is cheaper than the one it mirrors. - [Important] The
midRenameLevels()fixture still named the intent root the pre-rename way, soensureDimensionRootsminted a second one and the test passed: the failure mode had an unasserted live demonstration in the suite. Fixture corrected, the create batch is now pinned, and the split case became its own test that asserts the warn.
Published contract and in-code docs still described the removed behaviour:
- [Important] Six doc sites, none of them in the diff. Two actively instructed consumers to keep dual-spelling handling:
schemas.yaml"Match both spellings when keying on that dimension", andserenity-api.yaml"page_intentsalso claims the pre-renameintent__prefix". The other four areschemas.yamlSerenityPromptTagandpage_intents,handlers/prompts.jsbuildTagsOf, anddimensionOfRootName. All updated.
Blast radius was reducible without reintroducing tolerance:
- [Important] A pre-rename
intent__tag reached customers as a dynamicintentfilter group, because the reserved-key list is derived from the result's own keys and the intent key ispage_intents.intentis now reserved explicitly, so such a tag lands in the generictagsarray. It is still not read as an intent - this bounds the blast radius on an un-swept project rather than tolerating it. The catch-all now also drops anything carrying$abv_tags$, so the hiding control keys on the marker instead of a list that must be extended by hand.
Coverage lost or hollowed by the rewrite:
- [Important] A deleted rename-guard row covered behaviour that is still live: a value under a pre-rename
intentroot is still guarded as server-owned, because the fold is identity for that name. That property is what keeps an un-swept project safe rather than merely degraded, so it is asserted again. - [Important] The
excludes known-prefixed entriesassertion had become vacuous - its fixture no longer contains a bareintent__tag, so theintentcheck passed trivially while the name still advertised it. Retargeted, and the marker-drop behaviour got its own test. - [Important] The only remaining enrichment-failure signal was an unstructured warn, and the test covering that path builds its service without a logger, so
log?.warn?.was a no-op and the message went unverified. A blankuserIntentcannot otherwise be told from a failed call. The warn now carries aneventkey and is asserted against a real logger.
Non-blocking (11): all taken except two, noted below
- suggestion:
dimensionOfRootName's fold is correct and permanent - reframe the prose away from "a mid-rename project and a renamed one answer the same thing", do not remove the fold itself -src/support/serenity/prompt-tags.js:315 - suggestion: say out loud that the read and write paths now deliberately disagree about a bare
intentroot - the write path folds it to the intent dimension and guards its children as server-owned, while the read path surfaces those same tags as a customer-facing group. The asymmetry is the correct minimal mitigation, but an undocumented one invites a future reader to "fix" whichever side they meet first -src/support/serenity/prompt-tags.js:84 - suggestion:
missingNamesis a single-expression helper with one call site and a seven-line doc block; the indirection only existed to host the alias fold -src/support/serenity/tag-tree.js:118. Declined: pure churn on a branch already carrying a large deletion, and the named helper still reads better at the call site than an inline filter. - nit:
expect(log.info).to.not.have.been.calledguarded the adoption log that no longer exists; nothing logs info on this path now, so it only creates a failure vector the dayensureChildrengains one -test/support/serenity/tag-tree.test.js:357 - suggestion: gate the hidden family on the
$abv_tags$marker itself rather than enumerating one exact string, so the exposure control tracks the invariant instead of a list someone must remember to extend -src/support/elements/definitions/topics.js:213 - suggestion: the stated rationale for reserving bare
intentnames a threat that is unreachable through this API (no create or patch path can place a root-level customer tag). The reservation should stay, but its live value iscanonicalizeSourcerefusing a free-text source that folds onto a dimension name - state that instead -src/support/serenity/prompt-tags.js:88 - suggestion: the reservation survives only because
DIMENSION_PROVISION_ORDERis spread unmapped here while the other consumer maps it throughrootNameOfDimension. A future consistency cleanup would silently delete the guard - make the invariant structural -src/support/serenity/prompt-tags.js:97 - nit: the five-call test counts calls carrying the renamed prefix rather than asserting one per distinct intent value, so five calls for the same value would pass - assert the set. The companion
length(0)line is a real regression guard, not dead weight: a reintroduced legacy round would emitintent__-prefixed tags and trip it -test/support/elements/elements-service.test.js:365 - nit:
fetchIntentRoundnow has one call site with one possible argument and interpolates a constant into its warn - inline it -src/support/elements/elements-service.js:223 - nit: same stale bare-
intentfixture as item 3, in a second file -test/support/serenity/handlers/prompts.test.js:2759 - suggestion: the comment above the collapsed rename-guard table still reasons about two spellings being in play -
test/support/serenity/handlers/tags.test.js:1246. Kept as-is: the restored pre-rename row (item 6) puts two spellings genuinely back in play there, so the comment is accurate again.
One reviewer claim was not carried: that the length(0) line in the five-call test cannot fail. It can - a reintroduced legacy round emits intent__-prefixed tags, which the matcher catches. The real weakness was the companion count-only assertion, and that is what got fixed.
Out of scope, worth tracking
The same tolerance appears to exist in sibling repos this PR cannot gate: mysticat-data-service/scripts/serenity_migration (legacy_prefix, _adopt_legacy_authorship_root), and project-elmo-ui/src/utils/serenityTags.ts (findDimensionRoot) [unverified - surfaced by lexical search, not read]. If the API service drops tolerance first, an un-swept project reads inconsistently across the stack. Worth confirming whether LLMO-6986 covers all three and in what order.
Separately and pre-existing: transformOriginsToFilterDimensions populates origins from source__, while the tag tree provisions both an origin and a source root. Whether origin__* values could surface as a dynamic group beside origins could not be settled from the repo [unverified] - the fixtures use values matching neither vocabulary. Not caused by this PR.
|
Validated on the shared Subject:
Control, same request and project against main-based code earlier today: This confirms the degradation is contained but does not remove the sweep prerequisite: an un-swept project still reports no intents, so the rename must reach every project — dev included, as Separately confirmed live in the same response: |
…ent-root-tolerance # Conflicts: # src/support/elements/definitions/topics.js # src/support/serenity/prompt-tags.js # test/support/elements/definitions/topics.test.js
…ent-root-tolerance
…ent-root-tolerance
There was a problem hiding this comment.
Hey @rainer-friederich,
Verdict: Approve - clean simplification with thorough coverage; the external fleet-sweep gate is the real merge precondition, not the code.
Complexity: HIGH - medium diff; API surface (OpenAPI schema changes).
Changes: removes the dual-spelling tolerance for the intent dimension root so it is read and written only as $abv_tags$intent, adds marker-based filtering and blast-radius bounding for un-swept projects (14 files).
Note: Recommend a human read before merge - this change modifies shared contract documents (docs/openapi/schemas.yaml, docs/openapi/serenity-api.yaml). The bot review is a complement to, not a replacement for, a human read here.
Non-blocking (3): minor issues and suggestions
- suggestion: The split-root warning (
tag-tree.js:307) fires only whencreatedNames.includes(INTENT_ROOT_NAME), while the adjacent origin guardrail checksbyName.has(LEGACY_SOURCE_ROOT_NAME)unconditionally. The asymmetry is tested and intentional (the warn catches the moment of damage, not a known pre-existing state), but making it unconditional would mirror the origin pattern and surface the Chow Tai Fook dual-root case on every request rather than relying on the fleet sweep for detection. -src/support/serenity/tag-tree.js:307 - nit:
fetchIntentRoundis now a zero-argument closure with one call site after the retry logic was removed. The indirection existed to parameterize the root spelling. Worth inlining in a follow-up cleanup. -src/support/elements/elements-service.js:233 - nit: The
rootNameOfDimensionJSDoc says "Defined here, above RESERVED_ROOT_NAMES, because that list is derived through it" - a positional rationale that becomes stale if the function moves. -src/support/serenity/prompt-tags.js:80
Skill: pr-review | Model: us.anthropic.claude-opus-4-6-v1[1m] | Duration: 2m 6s | Cost: $9.11 | Commit: 275a4662592f1e99f7bc5cd0fedb22983433526a
If this code review was useful, please react with 👍. Otherwise, react with 👎.
- warn whenever a bare `intent` root sits beside `$abv_tags$intent`, not only on the pass that mints the marked one: the split outlives its creating call, and neither the rename nor the reshape can repair it unattended - start the intent-enrichment round as a promise instead of a single-use thunk - state the RESERVED_ROOT_NAMES dependency in `rootNameOfDimension`'s docs rather than the position it relies on Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ent-root-tolerance
|
Addressed the review in MysticatBot:
On the human-read recommendation: agreed, and the OpenAPI edits are the smaller half of why. The merge precondition in section 6 of the description is external to this diff and is not met yet. |
There was a problem hiding this comment.
Hey @rainer-friederich,
⚠ Degraded review - no spec document was found for this change (searched the PR links, the touched repos' docs, the architecture/guidelines docs, and linked Jira). This review covers code-level quality but could not validate the change against an agreed design, so confidence is reduced. Add a spec link (PR template section 4) and re-request review for a full-confidence pass.
Verdict: Approve - all prior non-blocking suggestions addressed cleanly; no new issues introduced.
Complexity: HIGH - medium diff; API surface (OpenAPI schema changes).
Changes: addresses the three non-blocking suggestions from the prior review - inlines the intent-round thunk, rewords a positional JSDoc rationale, and widens the split-root warn to fire on state rather than on mint (14 files).
Note: Recommend a human read before merge - this change modifies shared contract documents (docs/openapi/schemas.yaml, docs/openapi/serenity-api.yaml). The bot review is a complement to, not a replacement for, a human read here.
Previously flagged, now resolved
- Split-root warning now fires on state (
byName.has(DIMENSION.INTENT)) rather than only at mint time, mirroring the origin guardrail pattern fetchIntentRoundinlined asintentRound- a promise started at declaration, joined inPromise.allrootNameOfDimensionJSDoc states functional purpose ("a change to the mapping widens or narrows what a customer tag may not shadow") rather than positional rationale
Skill: pr-review | Model: us.anthropic.claude-opus-4-6-v1[1m] | Duration: 0m 57s | Cost: $5.05 | Commit: e0c8db79c96fd0a31e13b6b2138ef33a594fac10
If this code review was useful, please react with 👍. Otherwise, react with 👎.
…ent-root-tolerance
…ent-root-tolerance
…ent-root-tolerance
|
Validated on the shared A precondition first: dev had no project this change can read. All eight mapped dev projects carried a bare
One path is not exercised: the |
Fleet census — prod is clean, the tolerance has nothing left to tolerateRemoving the dual-spelling read means any project still carrying a bare Read-only census against prod (
Coverage is 484 of 484 mapped prod projects — every one resolved, all with Dev was swept separately and stands at 8 of 8, verified by reading each tag tree The read path was exercised end-to-end on dev against a renamed project — the The guardrail added here reports on state rather than on the moment of minting, |
## [1.755.2](v1.755.1...v1.755.2) (2026-08-19) ### Bug Fixes * **serenity:** read and write the intent root under one name | LLMO-6986 ([#3054](#3054)) ([4e256e8](4e256e8)), closes [Hi#level](https://github.com/Hi/issues/level)
|
🎉 This PR is included in version 1.755.2 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
1. Abstract
Resolves the Semrush AIO intent dimension root by its upstream name
$abv_tags$intentalone, on both the Elements read path and the tag-tree write path.2. Reasoning
The intent root was renamed project by project from the data-service migration CLI, so for the duration of that sweep both spellings had to be readable and the write path had to avoid minting a second root beside a not-yet-renamed one. Once no live project carries a bare
intentroot, that tolerance is cost without benefit — and a tolerant fallback nobody deletes is how thesource→originrename leftLEGACY_ORIGIN_DIMENSIONsitting inproject-elmo-uito this day.3. High-level overview of the changes
The enrichment costs five Elements calls, always.
getPromptsfires one intent-filtered call per intent value under$abv_tags$intent__and joins the rows back. The call count no longer depends on how a project is tagged: an all-empty round is simply an empty answer. Because nothing now infers "un-renamed" from an empty round, each call's failure no longer has to be distinguished from its emptiness, and the per-call result drops to{ key, rows }.The filter-dimension reader knows one intent prefix.
transformIntentsToFilterDimensionsandKNOWN_TAG_PREFIXESboth key on$abv_tags$intent__.The tag-tree resolver resolves roots by name.
ensureDimensionRootsprovisions and resolves the five roots asDIMENSION_PROVISION_ORDER.map(rootNameOfDimension), with no alias map.ensureChildren'saliasesparameter and itsadoptedNamesreturn field go with it — the alias mechanism was added for this rename and has no other caller, so what remains is a plain "which wanted names are absent" helper.The bare name stays reserved, deliberately.
RESERVED_ROOT_NAMESstill containsintent, and that is not leftover tolerance.dimensionOfRootNamemaps the bare name to the intent dimension, so a customer category namedintentat the root level would be read as the dimension itself. Reserving it keeps a customer from creating that collision. The comment now states that reason rather than the transitional one.A split dimension is reported as a state.
ensureDimensionRootswarns whenever a bareintentroot sits at the root level beside$abv_tags$intent, under the eventintent-rename-split-root. The condition is the presence of the pre-rename root, not the act of minting the marked one beside it — a mint-only check reports the moment a project breaks and is silent on every pass thereafter, which is backwards for a state that persists and that neither the rename nor the reshape can clear on its own. It mirrors thesource→originguardrail one block above, and it is what replaces the three transition log events removed here.4. Required information
5. Affected / used mysticat-workspace projects
scripts/backfill/intent_root_renameis what renames a project's root, and itsscripts/serenity_migrationreshape refuses a project still carrying the bare name. The sweep that has to come back empty is a sweep of what that tooling has reached.userIntent; on a project this change can no longer read, the tile renders as total misalignment.6. Additional information outside the code
This must not merge until a fleet sweep shows zero live projects with an
intent-named root. That is the ticket's first acceptance criterion, and the failure mode is worse than a degraded read. A project that still carries the bare root loses itsuserIntent, and its intent tags then fall through the filter-dimension catch-all and surface in the customer-facing Brand Presence filter as a dynamicintentgroup — reintroducing exactly the exposure the$abv_tags$marker exists to prevent. There is a test pinning that behaviour, so the consequence is visible rather than latent.The prod fleet is renamed but not clean. The sweep applied the rename to all 473 mapped projects on 2026-08-14, and a full acceptance re-run returned 473/473 already-renamed with zero writes. A rescan on 2026-08-18 over the by-then 477 mapped projects found four exceptions, each re-probed live against the Project Engine on 2026-08-18 and unchanged:
HK-zhcarries BOTH roots, in draft and published, with all 90 prompts dual-tagged. Its unmarked root is already visible in that customer's Brand Presence tag filter.enandzhcarry the bare root alone, in draft and published. These are the projects this change breaks.403 account_link_requiredand are proven neither way.All four were provisioned by api-service before LLMO-6984 named the root, and were absent from
brand_to_semrush_projectswhen the fleet sweep walked it — so the sweep's coverage was never the fleet. Dev is not exempt either: theBarkproject on dev still carries a bareintentroot, and running this branch against it on the shared dev slot reproduces the dynamic-group exposure described above.The three transition log events this change removes were the intended gate —
intent-rename-legacy-root-adopted,intent-rename-legacy-retry,intent-rename-legacy-tags-present. Anyone querying them for evidence should note they are emitted from api-service prod, whose logs go to CloudWatch rather than Splunk.intent-rename-split-rootsucceeds them, and it reports the harder state: a project carrying both roots at once, which the sweep's per-project rename refuses and therefore cannot fix.7. Test plan
The renamed-shape coverage is kept and the legacy-shape tests are removed rather than inverted, except where the inverse is now a real contract worth pinning: a bare
intent__tag must not be claimed as an intent, and the enrichment must cost five calls whether or not a project has intent tags.Before merging, in order:
intent_root_renamerefuses a project carrying both names, andreshape_project_tagsrefuses the bare one.intent. Attach the output to the ticket. The sweep itself needs fixing first —sweep_common.mapped_projectsstill selectsbrands.semrush_workspace_id, a column no longer in prod, so every sweep built on it dies withUndefinedColumn.userIntentis populated on the prompts response and the Topic Intent Alignment tile reads correctly.Step 3 needs a project with real weeks: the two projects renamed first both report
weeks: [], so they have no Brand Presence results to enrich.8. Deployment & merge order
$abv_tags$intentroot | LLMO-6984 #3043 (LLMO-6984) — already merged and released in v1.743.0. This PR deletes the tolerance that one added.feat/customer_onboardingvia https://github.com/adobe/mysticat-data-service/pull/902 — but merging tooling is not the same as having run it. The precondition is the sweep result, not a merge.spacecat-api-servicereleases to prod frommainvia semantic-release, so merging this IS the prod deploy. There is no staging window in which a wrong sweep result could be caught.The code is complete; the evidence in §7 is the gate. Do not merge on a green build alone.
🤖 Generated with Claude Code