feat(project-engine-mock): add native nested tag fixtures - #1916
feat(project-engine-mock): add native nested tag fixtures#1916byteclimber wants to merge 18 commits into
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
MysticatBot
left a comment
There was a problem hiding this comment.
Mysticat could not complete a verified review of this pull request.
This is a service-side gap, not a finding about your code: no binding APPROVE or REQUEST_CHANGES review could be produced and verified for the current commit.
- Current head:
2c0dc1fffe56f8e0f2cdf3f0b65aec4c947f4ee5 - Failure category:
completion_contract_invalid - Request ID:
db67e390-aab4-11f1-8c8c-c0a2b3abe261 - Cost: $4.36
- Duration: 0m 18s
Please re-request a review (e.g. push a new commit or re-request from the configured reviewer) to try again.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
This PR will trigger a minor release when merged. |
MysticatBot
left a comment
There was a problem hiding this comment.
Hey @byteclimber,
Verdict: Request changes - one blocking issue in the pagination handler.
Complexity: HIGH - large diff (798 lines, 7 files).
Changes: Adds canonical tag and hidden intent taxonomy fixtures to the Project Engine mock, introduces opt-in bounded sibling pagination, and extends test coverage for nested/child-only/raw provider tag shapes (7 files).
Must fix before merge
- [Important]
pagequery parameter not validated in pagination handler -packages/spacecat-shared-project-engine-client/mock/counterfact/routes/v2/workspaces/{id}/projects/{project_id}/aio/tags.js:133(details inline)
Non-blocking (5): minor issues and suggestions
- suggestion: Slice the matched set before serializing to avoid unnecessary work when paginating -
tags.js:136. Currentlymatched.map(serialize)runs on every item beforeslice()discards most of them. Slicingmatchedfirst then serializing only the page produces the same result with less work. - suggestion: Add an overlap/completeness assertion to the pagination E2E test -
project-engine-mock.e2e.js:1743. The test checks page sizes and the complete flag, but never verifies that the union of paged items equals the unpaged result set. Asserting item ids across pages would strengthen the contract. - nit: Pagination JSDoc paragraph (lines 43-45) reads as a continuation of the
path[]bullet rather than a new top-level behavior note -tags.js:43. - suggestion: Add a one-line comment to
CASE_VARIANT_ROOT_TAG_IDexplaining it is a case-variant collision fixture (capital-T "Tag"), not the dimension root (lowercase "tag") -seeds.js:186. - suggestion: Add a comment to
RAW_PROVIDER_TAGS_WORKSPACEnoting the spread-per-collection-key pattern, since a future contributor extending it to overlay a second collection would need to follow the same dance -seeds.js:487.
| // `page` arrives as a query string (e.g. "2"); coerce so the response field stays the numeric | ||
| // type AIOTagsListResponse declares, regardless of whether the param was passed. | ||
| return $.response[200].json({ items, page: Number(query?.page ?? 1), total: items.length }); | ||
| const page = Number(query?.page ?? 1); |
There was a problem hiding this comment.
issue (blocking): page parameter is not validated, allowing negative, zero, or NaN values to silently produce wrong results.
The limit parameter is carefully validated (must be integer 1-100), but page passes through Number() with no guard. When hasLimit is true:
page=0makesstart = (0 - 1) * limitnegative, andArray.prototype.slicewith a negative index selects elements from the end of the array - silently returning wrong data instead of an error.page=-1produces a larger negative start with similar wrong-data behavior.page=abcproducesNaN, makingstart = NaNandslice(NaN, NaN)returns[]. The response includespage: NaNin a typed numeric field.
This matters because a consumer testing their pagination logic against this mock could pass page=0 (a common off-by-one in zero-indexed implementations) and get a 200 with data from the wrong part of the list rather than a 400, leading to a false-green integration test.
Suggested fix - validate page the same way limit is validated when hasLimit is true:
const page = Number(query?.page ?? 1);
if (hasLimit && (!Number.isInteger(page) || page < 1)) {
return $.response[400].json(context.factories.createBasicResponseMock({
message: 'page must be a positive integer',
}));
}Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Hey @byteclimber,
Verdict: Request changes - one remaining gap in the pagination handler's no-limit branch.
Complexity: HIGH - large diff (348 additions, 7 files); API surface signal.
Changes: Adds canonical tag and hidden intent taxonomy fixtures to the Project Engine mock, introduces opt-in bounded sibling pagination with validation, and extends test coverage for nested/child-only/raw provider tag shapes (7 files).
Note: CI checks are currently pending - verify before merge.
Must fix before merge
- [Important]
pageleaks as NaN into 200 responses whenlimitis absent -tags.js:140(details inline)
Non-blocking (4): minor issues and suggestions
- nit: Pagination JSDoc paragraph (lines 45-47) reads as a continuation of the
path[]bullet rather than a new top-level behavior note -tags.js:45. - suggestion: Add a one-line comment to
CASE_VARIANT_ROOT_TAG_IDexplaining it is a case-variant collision fixture (capital-T "Tag"), not the dimension root (lowercase "tag") -seeds.js:186. - suggestion: Add a comment to
RAW_PROVIDER_TAGS_WORKSPACEnoting the spread-per-collection-key pattern, since a future contributor extending it to overlay a second collection would need to follow the same dance -seeds.js:487. - suggestion: Add a test for
pagebeyond last page (page=100, limit=2on a 5-item set) to anchor the empty-page-with-complete:true contract -project-engine-mock.e2e.js.
Previously flagged, now resolved
pagequery parameter now validated (integer, >= 1) whenhasLimitis true - 400 on invalid values, with E2E covering 0, -1, 1.5, and non-numeric strings.- Serialize-after-slice optimization applied -
matched.slice(start, start + limit).map(serialize)avoids serializing items outside the requested page. - Overlap/completeness assertion added - paged items checked for uniqueness (Set size) and membership equality against the unpaged result set.
Skill: pr-review | Model: us.anthropic.claude-opus-4-6-v1[1m] | Duration: 6m 18s | Cost: $10.36 | Commit: 23fea98164f87dd8add911f110fd79128f65d208
If this code review was useful, please react with 👍. Otherwise, react with 👎.
| })); | ||
| } | ||
| // Count the full parent/search-matched sibling set before selecting a requested page. | ||
| const total = matched.length; |
There was a problem hiding this comment.
issue (blocking): page leaks as NaN into 200 responses when limit is absent.
When a caller sends page=abc without limit, Number("abc") evaluates to NaN. The page validation guard at line 133 only fires when hasLimit is true, so the no-limit branch at line 140 returns { items: [...], page: NaN, total: N } with status 200. JSON.stringify serializes NaN as null, violating the AIOTagsListResponse contract where page is typed as an integer. A consumer that reads the returned page value for logging, pagination state, or assertion would see null instead of 1.
Suggested fix - hardcode page: 1 in the unpaged branch (it is always a single page), or validate page unconditionally before the hasLimit branch:
if (!hasLimit) {
return $.response[200].json({ items: matched.map(serialize), page: 1, total });
}Alternatively, move the page validation out of the hasLimit guard so it fires whenever page is explicitly provided.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Hey @byteclimber,
Verdict: Approve - prior blocking findings addressed, pagination logic correct, fixtures well-structured.
Complexity: HIGH - medium diff (454 lines, 7 files); API surface signal.
Changes: Adds canonical tag and hidden intent taxonomy fixtures to the Project Engine mock, introduces opt-in bounded sibling pagination with validation, and extends test coverage for nested/child-only/raw provider tag shapes (7 files).
Non-blocking (3): minor issues and suggestions
- suggestion: Add a one-line comment above the
RAW_PROVIDER_TAGS_WORKSPACEspread noting that the prompt collection is intentionally inherited unchanged fromWORKSPACE_WITH_DATAand that the case-variant root (Tag) is distinct from the dimension root (tag) -seeds.js:485. - suggestion: The long pagination E2E test (~70 lines, 6 scenarios) would benefit from short inline section comments (e.g.
// --- invalid page without limit ---,// --- paginated reads ---) to make each scenario scannable -project-engine-mock.e2e.js:1733. - suggestion: Add a boundary test for
limit=0alongside the existinglimit=101rejection test to round out the lower/upper bound pair -project-engine-mock.e2e.js.
Previously flagged, now resolved
pageparameter now validated (integer, >= 1) whenhasLimitis true, with E2E covering 0, -1, 1.5, and non-numeric strings.pagehardcoded to 1 in the unpaged branch, preventing NaN from leaking into 200 responses.- Serialize-after-slice optimization applied -
matched.slice(start, start + limit).map(serialize)avoids serializing items outside the requested page. - Overlap/completeness assertion added - paged items checked for uniqueness and membership equality against the unpaged result set.
- Beyond-last-page test added (page=100, limit=2 on a 5-item set returns empty items with complete:true).
Skill: pr-review | Model: us.anthropic.claude-opus-4-6-v1[1m] | Duration: 2m 11s | Cost: $5.62 | Commit: f38f3fd30214c08cef6a3fe3f81c39327b167cf9
If this code review was useful, please react with 👍. Otherwise, react with 👎.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Hey @byteclimber,
Verdict: Approve - prior blocking findings addressed, pagination logic correct, fixtures well-structured.
Complexity: HIGH - large diff (936 lines, 8 files); API surface signal.
Changes: Adds canonical tag and hidden intent taxonomy fixtures to the Project Engine mock, introduces opt-in bounded sibling pagination with validation, removes the undeclared complete field, and extends test coverage for nested/child-only/raw provider tag shapes (8 files).
Note: CI checks are currently pending - verify before merge.
Non-blocking (2): minor issues and suggestions
- nit: The mock-usage.md GET /aio/tags row says "without limit, the complete unpaged sibling set is returned with page: 1" which could imply page still takes effect in unpaged mode. A parenthetical like "(any page param is ignored)" would clarify the documented contract -
packages/spacecat-shared-project-engine-client/docs/mock-usage.md:132. - suggestion: The filtered pagination E2E test ("reports filtered tag pagination totals and items") verifies page 1 of the search=Re result but never fetches page 2 to confirm the second match comes through. The unfiltered pagination test already exercises the overlap/completeness check, so this is low risk, but a page-2 fetch would close the loop -
packages/spacecat-shared-project-engine-client/test/e2e/project-engine-mock.e2e.js:1844.
Previously flagged, now resolved
- Page query parameter now validated (integer, >= 1) when hasLimit is true, with E2E covering 0, -1, 1.5, and non-numeric strings.
- Page hardcoded to 1 in the unpaged branch, preventing NaN from leaking into 200 responses.
- Root-membership assertion changed from .include.members to .have.members (exhaustive, exact-set, no-extras guard).
- Undeclared complete field removed from paginated response - no provenance in vendored swagger or overlay corrections.
- mock-usage.md updated: seed count, dimension root names, pagination contract, new raw-provider-tags seed description.
Skill: pr-review | Model: us.anthropic.claude-opus-4-6-v1[1m] | Duration: 1m 56s | Cost: $9.25 | Commit: 4cfcb5472b1fad8f61795129651b0f123b176d69
If this code review was useful, please react with 👍. Otherwise, react with 👎.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
MysticatBot
left a comment
There was a problem hiding this comment.
Hey @byteclimber,
Verdict: Request changes - one blocking documentation gap in the raw-provider-tags seed.
Complexity: HIGH - large diff (963 lines, 8 files); API surface signal.
Changes: Adds canonical tag and hidden $abv_tags$intent taxonomy fixtures to the Project Engine mock, introduces opt-in bounded sibling pagination with validation, and extends test coverage for nested/child-only/raw provider tag shapes (8 files).
Note: CI checks are currently failing (unrelated - tokowaka-client package).
Must fix before merge
- [Important] Raw-provider-tags seed SEED_IDS not documented or grouped -
packages/spacecat-shared-project-engine-client/mock/seeds.js:679(details inline)
Non-blocking (6): minor issues and suggestions
- suggestion: Move the
buildTagViewcall to after both limit/page validation gates so invalid requests skip the tag-view computation -tags.js:123. - suggestion: Add a one-line comment in
legacySlugDimensionRootTreenoting that thetagroot is intentionally absent (the slug-rename fixture predates the tag dimension) to explain the different root cardinality from other seeds -seeds.js:354. - nit: Line break after "with no
parent_id, and" leaves a dangling conjunction at the start of the next line - reflow the sentence -tags.js:17. - suggestion: Add a filtered-pagination page 2 fetch (
search=Re&limit=1&page=2) to verify the second match comes through and close the filter + page boundary contract -project-engine-mock.e2e.js:655. - suggestion: Add
limit=-1andlimit=1.5to the invalid-limits E2E test for symmetry with the page validation test coverage -project-engine-mock.e2e.js:627. - nit: Completeness-signal assertions use
page * 2instead of a namedLIMITconstant -project-engine-mock.e2e.js:573.
Previously flagged, now resolved
pagequery parameter now validated (integer, >= 1) whenhasLimitis true, with E2E covering 0, -1, 1.5, and non-numeric strings.pagehardcoded to 1 in the unpaged branch, preventing NaN from leaking into 200 responses.- Root-membership assertion changed from
.include.membersto.have.members(exhaustive, exact-set, no-extras guard). - Undeclared
completeresponse field removed from paginated response - no provenance in vendored swagger or overlay corrections. docs/mock-usage.mdseed count, dimension root names, and pagination contract all updated correctly.SEED_IDSenumeration now includeschildOnlyPromptId,tagParentTagId,tagChildTagId, and says "the six*RootTagIds".
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Hey @byteclimber,
Verdict: Approve - prior blocking findings all addressed, pagination logic correct, fixtures well-structured.
Complexity: HIGH - large diff (1011 lines, 8 files); API surface signal.
Changes: Adds canonical tag and hidden $abv_tags$intent taxonomy fixtures to the Project Engine mock, introduces opt-in bounded sibling pagination with validation, adds a raw-provider-tags seed for classification tests, and extends test coverage across unit, stateful, tag-view, and E2E layers (8 files).
Note: CI checks are currently pending - verify before merge.
Non-blocking (4): minor issues and suggestions
- nit: Pagination JSDoc paragraph (lines 45-47 in tags.js) reads as a continuation of the prior bullet rather than a new top-level behavior note - a blank JSDoc line or a dedicated bullet prefix would clarify scope -
tags.js:45. - suggestion: Extract the raw-provider tag string literals (
Road-Running,Road Running,Men/Women,Deep,Nested,Unsupported) into named constants matching the existingTAG_PARENT_NAME/TAG_CHILD_NAMEpattern to prevent the id-constant and seed-entry copies from silently diverging -seeds.js:179-190. - nit:
CHILD_ONLY_PROMPT_IDatseeds.js:62lacks the trailing inline comment (// AIOPromptWithStatus.id) that every other id constant in the block carries. - suggestion:
Number()coercion onlimitandpageaccepts scientific notation (1e2) and hex (0x10) as valid integers. For a mock this is cosmetic, butparseInt(s, 10)with a/^\d+$/pre-check would match a stricter wire contract if desired -tags.js:118.
Previously flagged, now resolved
pagequery parameter now validated (integer, >= 1) whenhasLimitis true, with E2E covering 0, -1, 1.5, and non-numeric strings.pagehardcoded to 1 in the unpaged branch, preventing NaN from leaking into 200 responses.- Root-membership assertion changed from
.include.membersto.have.members(exhaustive, exact-set, no-extras guard). - Undeclared
completeresponse field removed from paginated response - no provenance in vendored swagger or overlay corrections. docs/mock-usage.mdupdated: seed count, dimension root names, pagination contract, new raw-provider-tags seed description with ids.SEED_IDSenumeration now includeschildOnlyPromptId,tagParentTagId,tagChildTagId, and says "the six*RootTagIds".
Skill: pr-review | Model: us.anthropic.claude-opus-4-6-v1[1m] | Duration: 1m 36s | Cost: $8.76 | Commit: f35b3c56aebb5e1d84f0325932f29ad7f4489814
If this code review was useful, please react with 👍. Otherwise, react with 👎.
Summary
tagand hidden$abv_tags$intenttaxonomy fixtures while preserving independentoriginandsourceContract: adobe/serenity-docs#26 at
4fb3a76. Adobe compatibility DTO classification remains owned byspacecat-api-service; this package exposes provider-shaped data only.Validation
Package publication is intentionally deferred to human checkpoint 2.