Skip to content

feat(project-engine-client): add tag: dimension mock fixtures#1780

Open
byteclimber wants to merge 4 commits into
mainfrom
feat/plain-tags-mock-fixtures
Open

feat(project-engine-client): add tag: dimension mock fixtures#1780
byteclimber wants to merge 4 commits into
mainfrom
feat/plain-tags-mock-fixtures

Conversation

@byteclimber

Copy link
Copy Markdown
Contributor

1. Abstract

Adds seed/fixture support for the tag: open tag dimension to the spacecat-shared-project-engine-client Counterfact mock. tag: is a sibling of the existing category: dimension (serenity-docs#26) — the same 1-level-tree shape (a tag:<name> root owning bare parent_id-linked children), living in the same per-project tags collection. This is a deliberately narrow-scope, fixtures-only change.

2. Reasoning

The Semrush "serenity" AIO tag taxonomy exposes an OPEN <dimension>: prefix. category: is already modelled end-to-end in the mock; tag: is a new sibling dimension (serenity-docs#26) that the api-service tag-dimension work needs to read back from seeded projects. Rather than hand-write literals, the mock's factory/seed machinery is extended so a tag: root + child seed exactly mirrors the category: fixture and is usable in cross-repo integration tests the same way.

3. High-level overview of the changes

  • mock/factories.jscreateAIOTagMock's doc comment now documents the tag: dimension alongside category:: they are sibling OPEN dimensions with the same shape (not sub-types of one another), and the factory is dimension-agnostic (it only stores name/parent_id; the dimension lives in the name prefix). No code change — the factory already builds either dimension.
  • mock/seeds.js — adds TAG_ROOT_NAME/TAG_CHILD_NAME/TAG_ROOT_TAG_ID/TAG_CHILD_TAG_ID mirroring the CATEGORY_* constants; threads a new tagTags param through peHierarchy (concatenated into the one per-project tags collection next to categoryTags); seeds a tag: root + bare child in WORKSPACE_WITH_DATA and a flat tag: list in the second two-hierarchies hierarchy; exposes tagRootTagId/tagChildTagId on SEED_IDS. The tag: child name is deliberately DISTINCT from the category: child (Ultra vs Trail) to avoid the name-alone tag-id collapse (see below).
  • Pending-probe comments (no behaviour change) in three places that describe the sha256(name)-alone id-derivation limitation, cross-referencing each other and the serenity-docs#26 §9 G1 probe: mock/tag-id.js, mock/stateful.js (ops.tags.upsertMany), and the aio/tags route header.
  • Tests — extends test/mock/factories.test.js (proves createAIOTagMock builds a tag: root/child dimension-agnostically) and test/mock/seeds.test.js (asserts both dimensions coexist in the seeded tags collection with correct root/child parenting).

Deliberately NOT in this PR (scope gate)

The known sha256(name)-alone id-derivation bug — which would cause a same-named child under two DIFFERENT roots to collapse to ONE mock id — is not fixed here. It is gated on an unrun live probe (serenity-docs#26 §9 G1) confirming whether real Semrush actually salts a child tag's id by parent_id or keys by name alone. Fixing the mock before that probe runs would bet on the unconfirmed answer and make the mock diverge from reality in exactly the way that would hide the bug the probe exists to catch. The three pending-probe comments flag this so a future contributor fixing one spot doesn't miss the others.

There is also no GET .../aio/tags/{tag_id} single-tag-by-id operation (only PATCH) — confirmed absent in both the swagger and the mock routes; this is a known, accepted gap, not touched here.

4. Required information

5. Affected / used mysticat-workspace projects

  • spacecat-api-service — the matching tag: dimension work depends on this fixture. Its integration tests will need to bump their package.json dependency on @adobe/spacecat-shared-project-engine-client once this is released/versioned, to pick up the new tag: seed fixture.

6. Additional information outside the code

  • The mock's stateful ops round-trip the new fixture (covered by the extended seeds.test.js). The full package unit suite, tsc type-check (// @ts-check gate), and lint were run locally.

7. Test plan

  • (a) Local: npm run lint, npm run test:types, and npm test for the package — all green, 100% coverage retained (branches 100 as the package enforces).
  • (b) Per-env: this package ships no runtime service; after merge, semantic-release publishes a new npm version and the GHCR mock image republishes on the release tag. Consumers verify by bumping the dependency and reading the tag: fixture (SEED_IDS.tagRootTagId/tagChildTagId) in their integration tests.

8. Deployment & merge order

  • This PR is a dependency for the matching spacecat-api-service tag:-dimension PR. After this merges and releases, api-service bumps its @adobe/spacecat-shared-project-engine-client dependency to pick up the new fixture.

🤖 Generated with Claude Code

Adds seed/fixture support for the `tag:` open tag dimension, a sibling of
`category:` with the same 1-level-tree shape (serenity-docs#26). The mock's
`createAIOTagMock` is already dimension-agnostic; this wires a `tag:` root +
bare child through `peHierarchy`/`buildSeed` into both seed hierarchies and
`SEED_IDS`, and documents `tag:` alongside `category:`.

Deliberately narrow scope: does NOT fix the `sha256(name)`-alone tag-id
derivation (gated on the unrun serenity-docs#26 §9 G1 live probe). Adds
cross-referencing "pending live-probe confirmation" comments in tag-id.js,
stateful.js, and the aio/tags route header.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@byteclimber
byteclimber requested a review from MysticatBot July 6, 2026 10:17
@byteclimber
byteclimber marked this pull request as ready for review July 6, 2026 11:16
@byteclimber
byteclimber requested review from MysticatBot and removed request for MysticatBot July 6, 2026 11:17

@MysticatBot MysticatBot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Hey @byteclimber,

Verdict: Request changes - E2E tests break because the new seed data is visible to existing root-listing assertions.
Complexity: LOW - small fixtures-only diff, single package.
Changes: Adds tag: dimension seed fixtures (root + child) alongside existing category: fixtures in the project-engine-client Counterfact mock (7 files).
Note: CI checks are currently failing - resolve before merge.

Must fix before merge

  1. [Critical] E2E tests not updated for new tag: root in seed - 6 assertions fail - mock/seeds.js:221 (details inline)
  2. [Important] TWO_HIERARCHIES seed's new tagTags has no test assertions - mock/seeds.js:299 (details inline)
Non-blocking (1): minor issues and suggestions
  • nit: SeedProject typedef's tags description still only mentions category: but the collection now holds multiple dimensions - mock/seeds.js:~116

Skill: pr-review | Model: us.anthropic.claude-opus-4-6-v1[1m] | Duration: 1m 42s | Cost: $7.04 | Commit: 2a9ba34e8904ef32d064af242a066c1db7b838a1
If this code review was useful, please react with 👍. Otherwise, react with 👎.

@@ -208,7 +221,7 @@ const peHierarchy = (cfg) => buildSeed({
domain: cfg.domain,

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

issue (blocking): The spread [...cfg.categoryTags, ...cfg.tagTags] adds the new tag:Trail Running root to WORKSPACE_WITH_DATA. The E2E test suite (project-engine-mock.e2e.js) queries roots via parent_id: "" and hardcodes expected counts/names based on only category: roots existing.

6 failing E2E assertions:

  • Line 835: expects total: 2, gets 3 (tag:Trail Running root now included)
  • Line 873: expects total: 3, gets 4 (same root-count off-by-one)
  • Line 910: expects total: 1 after reset, gets 2 (both seeded roots remain)
  • Lines 1013/1098: have.members lists only category: roots, actual includes tag:Trail Running
  • Line 1225: expects only category:Running Shoes after delete, gets tag:Trail Running too

Fix: Update the 6 E2E test assertions to include the new tag:Trail Running root in their expected counts and member lists. Also rebase onto main and regenerate package-lock.json (lockfile drift is causing the Test job to fail independently).

benchmarkId: 'e7f8a9b0-c1d2-4e3f-8a4b-6c7d8e9f0112',
brandUrlId: 'f8a9b0c1-d2e3-4f4a-8b5c-7d8e9f011223',
categoryTags: aioTags(['category:Laufschuhe', 'category:Trailrunning']),
tagTags: aioTags(['tag:Laufschuhe', 'tag:Trailrunning']),

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

issue (blocking): The second hierarchy gains tagTags: aioTags(["tag:Laufschuhe", "tag:Trailrunning"]) but the two-hierarchies test block in seeds.test.js is not updated to assert that these tags appear in the collection. The WORKSPACE_WITH_DATA path gets thorough assertions (root/child structure, parent_id linkage, SEED_IDS verification), but the second hierarchy's new tags are unverified.

Fix: Add assertions in the two-hierarchies test block verifying ops.tags.list(...) for the second hierarchy includes the tag:Laufschuhe and tag:Trailrunning entries alongside the existing category tags.

@MysticatBot MysticatBot added ai-reviewed Reviewed by AI complexity:low AI-assessed PR complexity: LOW labels Jul 6, 2026
…or tag: seed

Addresses MysticatBot review on #1780:
- Critical: the boot seed's new `tag:Trail Running` root now appears in every
  empty-parent_id root listing, so the 6 E2E assertions that asserted an exact
  root set are updated to include it; adds an E2E test reading the baked tag:
  nested taxonomy (root -> child Ultra) end-to-end.
- Important: TWO_HIERARCHIES seed's tagTags now has unit assertions.
- Nit: SeedProject `tags` typedef documents both open dimensions.
- Also syncs package-lock.json (data-access 4.1.0) to satisfy the CI drift gate.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@github-actions

github-actions Bot commented Jul 6, 2026

Copy link
Copy Markdown

This PR will trigger a minor release when merged.

@byteclimber
byteclimber requested a review from MysticatBot July 6, 2026 11:43

@MysticatBot MysticatBot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Hey @byteclimber,

Verdict: Approve - all prior findings addressed, clean re-review.
Complexity: HIGH - medium diff; dependency signal from lockfile change.
Changes: Adds tag: dimension seed fixtures (root + child) alongside existing category: fixtures in the project-engine-client Counterfact mock (9 files).

Previously flagged, now resolved

  • E2E assertions now account for the new tag:Trail Running root across all 6 root-listing scenarios
  • TWO_HIERARCHIES seed now has dedicated test assertions verifying both dimensions coexist
  • SeedProject typedef description updated to mention both open tag dimensions
Non-blocking (0): no issues found

No issues identified on re-review. The fix commit correctly bumps all assertion counts, adds a dedicated E2E test for the tag: nested taxonomy read-back, and adds TWO_HIERARCHIES coverage for both dimensions. Test coverage is thorough across all three levels (factory unit, seed integration, E2E contract).


Skill: pr-review | Model: us.anthropic.claude-opus-4-6-v1[1m] | Duration: 0m 58s | Cost: $4.53 | Commit: 521b67786bfe6a099985e7df79aa88e71b79bcf6
If this code review was useful, please react with 👍. Otherwise, react with 👎.

@MysticatBot MysticatBot added complexity:high High complexity PR and removed complexity:low AI-assessed PR complexity: LOW labels Jul 6, 2026
@byteclimber
byteclimber requested a review from MysticatBot July 7, 2026 11:58

@MysticatBot MysticatBot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Hey @byteclimber,

Verdict: Approve - clean re-review, no new issues introduced by rebase.
Complexity: HIGH - medium diff; API surface signal (routes path pattern).
Changes: Adds tag: dimension seed fixtures (root + child) alongside existing category: fixtures in the project-engine-client Counterfact mock (8 files).

Previously flagged, now resolved

  • E2E assertions now account for the new tag:Trail Running root across all 6 root-listing scenarios
  • TWO_HIERARCHIES seed now has dedicated test assertions verifying both dimensions coexist
  • SeedProject typedef description updated to mention both open tag dimensions
Non-blocking (2): minor issues and suggestions
  • nit: seeds.test.js assertion uses deep.equal on tag name array, coupling to insertion order - if order is not a contract, have.members would be more resilient - test/mock/seeds.test.js:80
  • nit: tag-id.js pending-probe comment references "two other spots" by description only - grep-friendly but could benefit from the function/symbol names inline (already partially done via ops.tags.upsertMany) - mock/tag-id.js:~37

Skill: pr-review | Model: us.anthropic.claude-opus-4-6-v1[1m] | Duration: 0m 53s | Cost: $2.97 | Commit: 6ca5c0061b80b07adf0590b2e2eb0b8af0ed8cdb
If this code review was useful, please react with 👍. Otherwise, react with 👎.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ai-reviewed Reviewed by AI complexity:high High complexity PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants