feat(erli): Erli-owned Allegro client_credentials category-catalog client

[norbert-kulus-blockydevs]

Summary

• Adds AllegroCategoryCatalogClient (libs/integrations/erli/src/infrastructure/http/allegro-category-catalog-client.ts), a self-contained HTTP client that lets an Erli connection browse Allegro's public /sale/categories and /sale/categories/{id}/parameters catalog via an Allegro app's grant_type=client_credentials token — no seller/user OAuth context and no dependency on @openlinker/integrations-allegro (ADR-030).
• Extends ErliConnectionConfig with an optional allegroEnvironment field and ErliCredentials with optional allegroClientId / allegroClientSecret fields (libs/integrations/erli/src/domain/types/erli-connection.types.ts).
• Adds unit tests covering token acquisition, the proactive-refresh-window cache, token re-acquisition after expiry, category/parameter response mapping, and typed-exception classification for a rejected token response and a network failure.

Part of #1381, sub-task 1 of 3 (see #1383, #1384).

This sub-task is intentionally standalone — it does not wire the client into ErliOfferManagerAdapter / ErliAdapterFactory yet (that's #1383).

Test plan

• pnpm --filter @openlinker/integrations-erli lint
• pnpm --filter @openlinker/integrations-erli type-check
• pnpm --filter @openlinker/integrations-erli test (17 suites / 304 tests, including the new spec)
• pnpm check:invariants (cross-context import check — no violations)

🤖 Generated with Claude Code

https://claude.ai/code/session_01RpsVFTpKZ1HoowEUKzbnWu

[norbert-kulus-blockydevs]

Review: AllegroCategoryCatalogClient (#1382, sub-task 1/3 of epic #1381)

Reviewed against docs/code-review-guide.md, docs/engineering-standards.md, ADR-030, and the implementation plan (docs/plans/implementation-plan-erli-allegro-category-catalog.md, currently still in unmerged PR #1380 — see doc-gap note below). Confirmed this is intentionally standalone (no wiring into ErliOfferManagerAdapter/ErliAdapterFactory yet — that's #1383) and reviewed it as such.

What I checked

• ADR-030 cross-plugin boundary: confirmed the new file imports only @openlinker/core/listings (type-only) plus this package's own exception classes — no import from @openlinker/integrations-allegro anywhere in the diff. Compliant.
• Token acquisition/caching: ensureToken() reuses the cached token while Date.now() < expiresAt - TOKEN_REFRESH_WINDOW_MS (60s), otherwise proactively re-acquires. No refresh_token handling (correct — client-credentials tokens don't have one). Verified against the unit tests, including the Date.now()-mocked expiry test.
• Field mapping fidelity: diffed toNeutralCategoryParameter/fetchCategories in this file line-by-line against AllegroOfferManagerAdapter.fetchCategories and libs/integrations/allegro/src/infrastructure/mappers/allegro-category-parameter.mapper.ts. The mapping is a faithful, field-for-field copy (dictionary entries, multiValue roll-up, dependsOn/dependsOnValueIds, section) — not a re-guess.
• Exception handling: network failures (fetch throws) and non-2xx responses are both caught and converted to typed exceptions (ErliNetworkException / ErliAuthenticationException), nothing is silently swallowed. response.text().catch(() => '') degrades gracefully if the error body itself isn't readable.
• Test quality: the spec genuinely exercises cache-hit (should reuse the cached token within the freshness window), cache-miss/first-acquisition, and expiry-triggered re-acquisition (via jest.spyOn(Date, 'now')), plus token-rejection and network-failure branches, plus category/parameter mapping including the dependsOn union-of-dictionary-values case. Not happy-path-only.
• Ran pnpm --filter @openlinker/integrations-erli lint on the PR branch in an isolated worktree — clean, no errors.
• Naming/placement/file headers/hexagonal layering: plain class in infrastructure/http/, mirrors ErliHttpClient's "plain class, constructed per-connection" shape, correctly kept out of the package's public barrel (same as ErliHttpClient/its interface).

Findings

IMPORTANT — libs/integrations/erli/src/infrastructure/http/allegro-category-catalog-client.ts
Wire-shape types (AllegroTokenResponse, AllegroCategoryItem, AllegroCategoriesResponse, AllegroCategoryParameter, AllegroCategoryParametersResponse, CachedToken) are defined inline in the client file rather than in a sibling *.types.ts. This is both the documented rule in engineering-standards.md § "Type Definitions in Separate Files" and the convention already followed one file over — erli-http-client.ts extracts its own wire/config types into erli-http-client.types.ts in the same directory, and Allegro's own equivalent types live in domain/types/allegro-api.types.ts. Recommend extracting these into allegro-category-catalog-client.types.ts for consistency with both siblings.

SUGGESTION — token acquisition has no single-flight guard
Two concurrent calls into ensureToken() when no token is cached will both fire acquireToken() (duplicate token requests, no correctness bug, just avoidable traffic). The reference pattern this design mirrors (AllegroConnectionTokenState) explicitly has single-flight-refresh; the plan calls this a "simplified" re-implementation, but it's worth a one-line comment noting this trade-off (or a light in-flight-promise guard) before #1383 puts real per-connection traffic through it.

SUGGESTION — test gap: 401/403 branch in requestJson (data calls) is untested directly
The auth-rejection tests only exercise acquireToken()'s branch. The sibling branch inside requestJson() (a 401/403 on /sale/categories or /sale/categories/{id}/parameters itself, e.g. an expired-but-not-yet-refreshed token rejected server-side) is the same code path but isn't exercised by a dedicated test.

SUGGESTION — reusing ErliAuthenticationException for Allegro credential rejections
Functionally fine (the plan explicitly sanctions checking whether an existing exception "fits" before adding a new one), and it does fit. Just flagging for #1383: this exception's file header is scoped to "Erli's static API key" and the connection's own auth-failure-classifier framing (#984/#819) — since this client authenticates with a different credential pair on the same connection, worth double-checking in #1383 that classifier wiring doesn't conflate an Allegro-app-credential rejection with an Erli-API-key rejection for needs_reauth purposes.

SUGGESTION (minor, edge case) — CachedToken.expiresAt: number | undefined: if Allegro's token response ever omits expires_in, ensureToken() treats the cached token as never-expiring. Unlikely in practice, but a defensive fallback (e.g. treat missing expires_in as already-expired) would be safer than caching indefinitely.

Documentation gap (not a code issue)

ADR-030 and the implementation plan referenced in the PR description aren't actually present on main or on the epic/1381-… base branch yet — they currently live only in still-open PR #1380. Worth merging #1380 before/alongside this stack lands so the referenced docs actually resolve for future readers of this PR.

Verdict

No blocking issues. Architecture boundary (ADR-030), token logic, and mapping fidelity are all correct and verified against source. The one IMPORTANT item (extract inline types to *.types.ts) is a quick, mechanical fix and doesn't change behavior. Suggestions are forward-looking notes for #1383, not required for this sub-task. Good to merge into the epic branch as-is, or with the types-file extraction applied.

[piotrswierzy]

/pr-review — feat(erli): AllegroCategoryCatalogClient (epic #1381, sub-task 1/3)

1️⃣ Summary

A self-contained Allegro client_credentials catalog client for Erli + the optional allegroClientId/allegroClientSecret/allegroEnvironment type fields. Deliberately standalone — no adapter/factory wiring (that's #1383). Quality is high: the ADR-030 plugin-independence boundary is respected and documented, the response mapping is a faithful copy of Allegro's real mapper (verified — no drift today), exceptions are typed, no any, excellent file header, and the token lifecycle is unit-tested. Correct in isolation. My two should-address items are about keeping it correct over time and the token cache once it's wired — neither blocks this building-block PR.

2️⃣ Alignment with docs — strong

• ADR-030 / plugin independence: no @openlinker/integrations-allegro import — only neutral types from @openlinker/core/listings + own domain/exceptions/ (verified). Header documents the decision.
• Hexagonal: plain HTTP client in infrastructure/http/; type fields in domain/types/ (so the app-layer factory can depend without inverting layers — matches the Allegro/PS layout). No NestJS/DI, per the ErliHttpClient precedent.
• Error handling: network → ErliNetworkException, token/401/403 → ErliAuthenticationException; no secret in messages. No any; file header present.
• Mapping fidelity (verified): toNeutralCategoryParameter matches allegro-category-parameter.mapper.ts field-for-field — same multiValue roll-up (multipleChoices || allowedNumberOfValues>1), section = describesProduct ? 'product' : 'offer' (#415), dependsOn/visibilityValueIds, customValuesEnabled. OfferCategory {id,name,parentId,leaf} matches.

3️⃣ Issues & Risks

🟡 IMPORTANT — the manually-copied mapper is a latent drift trap with no guard

toNeutralCategoryParameter (+ toNeutralEntry/unionEntryParentValues) is a hand-copy of libs/integrations/allegro/src/infrastructure/mappers/allegro-category-parameter.mapper.ts — correct today, but the file header's own "kept in sync manually per ADR-030" is the fragility. When Allegro's mapper next changes (the way #1035 added multiValue and #415 added section), this copy silently diverges and Erli offers get subtly-wrong required parameters — precisely the bug class this whole epic exists to fix, and the same duplication-drift shape as #917/#1365. ADR-030 accepted the duplication; it didn't provide a guard. Ask: add a golden-fixture parity test — a shared raw-parameter JSON fixture asserted to map identically by both mappers (or a checked-in expected-output snapshot both packages compare against), plus a reciprocal "change one → change the other" comment in both files. Cheap, and it converts a silent drift into a failing test.

🟡 IMPORTANT (forward-looking, for #1383 — not this PR) — the per-instance token cache will be per-request once wired

The token is cached on this.cached. Per my deep-review of the plan (#1380), ErliAdapterFactory builds the adapter — and thus this client — fresh on every getCapabilityAdapter call (integrations.service.ts:124, no memoization), and the /parameters endpoint is uncached at the controller layer. So once #1383 wires it, every parameter fetch pays a fresh client_credentials token round-trip. This client is correct in isolation; flagging so #1383 caches the app token at a shared layer (module-level keyed by clientId, or HostServices.cache) rather than relying on this instance field. Allegro's own adapter avoids this via a shared token state + the global allegro:cat-params:{categoryId} cache.

🟢 SUGGESTIONS

1. ensureToken — when expires_in is absent, expiresAt is undefined and the token is served indefinitely (this.cached.expiresAt === undefined short-circuits). Allegro always returns expires_in, so it's theoretical, but a missing/zero value would pin a possibly-expired token. Safer: treat undefined as "always re-acquire" (or a conservative default TTL).
2. No single-flight on acquireToken — two concurrent calls (fetchCategories + fetchCategoryParameters in parallel) on a stale token both hit /auth/oauth/token. Harmless (last-write-wins on this.cached), just a redundant fetch; a shared in-flight promise closes it. The ADR cited AllegroConnectionTokenState's single-flight as the reference — noting the simplification dropped it.
3. allegroEnvironment?: 'sandbox' | 'production' is an inline union; engineering-standards prefers as const + a Values array for domain constants (and #1383's config-shape validator will want the runtime array anyway). Mirrors AllegroConnectionConfig.environment, so minor.

6️⃣ Tests

Unit spec (281 lines) covers token acquisition, the refresh-window cache hit, re-acquisition after expiry, category + parameter mapping, and typed-exception classification for rejected-token + network failure — good AAA coverage of the client in isolation. The gap is the cross-mapper parity test called out above.

Final Verdict

🔄 Approve with changes — a clean, well-scoped standalone building block that's correct today. Land the mapper-parity drift guard (IMPORTANT #1) before or alongside the wiring PR, and carry the shared token-cache note (IMPORTANT #2) into #1383. Suggestions are optional polish. No blocking issues.

[norbert-kulus-blockydevs]

Addressed all findings from both reviews (IMPORTANT + SUGGESTION) in 306299a:

IMPORTANT

• Extracted the inline wire-shape types (AllegroTokenResponse, AllegroCategoryItem, AllegroCategoriesResponse, AllegroCategoryParameter, AllegroCategoryParametersResponse, CachedToken) into a sibling allegro-category-catalog-client.types.ts, matching the erli-http-client.ts / erli-http-client.types.ts split.
• Added a cross-plugin mapper parity test suite: the Erli spec now runs Allegro's real sandbox fixture (category-parameters-257933.json) through this client's copy of toNeutralCategoryParameter, mirroring the same field-level assertions (section, dependsOn, dictionary handling) as allegro-category-parameter.mapper.spec.ts. Cross-referencing comments were added in both spec files and in the mapper's own doc comment so a future change to either mapper prompts updating both. This can't be a fully automatic single-source-of-truth check without violating ADR-030 (no cross-plugin runtime import), but it converts today's silent-copy risk into a same-fixture, mirrored-assertion test pair.

SUGGESTIONS

• Added a single-flight guard on token acquisition (inFlightToken) so two concurrent fetchCategories/fetchCategoryParameters calls on a cold cache issue exactly one grant_type=client_credentials request.
• Added a dedicated test for the 401/403 branch on a data call (/sale/categories, /sale/categories/{id}/parameters) rejecting the bearer token, distinct from the already-covered token-endpoint rejection.
• Fixed the missing-expires_in edge case: a token response without expires_in is now treated as already-expired (always re-acquire) instead of being cached indefinitely, with a regression test.
• Replaced the inline 'sandbox' | 'production' union on ErliConnectionConfig.allegroEnvironment with an as const + runtime-array AllegroCatalogEnvironment type in erli-connection.types.ts, matching AllegroConnectionConfig's own convention; the client constructor now takes this type instead of an inline union.
• The ErliAuthenticationException reuse note and the shared-token-cache note are both explicitly forward-looking for [TASK] Integration — Erli adapter capability wiring + credentials/config validators for Allegro category access #1383 (adapter/factory wiring) per the reviews — no action needed in this standalone sub-task.

Verified: pnpm --filter @openlinker/integrations-erli lint, type-check, and test (17 suites / 312 tests, up from 304) all pass; node scripts/check-cross-context-imports.mjs reports no new violations.

[norbert-kulus-blockydevs]

Fresh re-review of the current diff (commit 306299a)

This is an independent re-review of the PR after the author's fix-up commit 306299a2 ("address PR #1388 review findings"), covering both a /tech-review pass and a full /pr-review pass. I re-read the full diff, both prior reviews (mine from the earlier round and Piotr's), and re-verified everything against the current code rather than trusting the "Addressed all findings" summary comment at face value.

Verification method

• Read the full current diff (gh pr diff 1388).
• Read both prior review bodies in full (my earlier review + Piotr's /pr-review) and cross-checked each finding against the current code.
• Checked out the existing worktree for this branch, confirmed it's synced to 306299a2 (matches origin/1382-erli-allegro-catalog-client).
• Ran the actual quality gate in that worktree (not just read the code):
  • pnpm --filter @openlinker/integrations-erli lint → clean.
  • pnpm --filter @openlinker/integrations-erli type-check → clean.
  • pnpm --filter @openlinker/integrations-erli test → 17 suites / 312 tests, all passing — matches the author's claimed numbers exactly.
  • node scripts/check-cross-context-imports.mjs → 1473 cross-context import(s) across 1928 files, all conform — no new violations.
• Verified the fixture path in the new mapper-parity test resolves to a real file: libs/integrations/allegro/src/infrastructure/adapters/__fixtures__/category-parameters-257933.json exists.
• Verified ErliAuthenticationException(message, statusCode?, url?) and ErliNetworkException(message, cause?) constructor signatures match the new call sites.

What was raised before vs. what's now fixed

IMPORTANT — wire-shape types inline (mine). ✅ Fixed. Extracted into allegro-category-catalog-client.types.ts, matching the erli-http-client.ts/.types.ts sibling split, per engineering-standards.md § "Type Definitions in Separate Files".

IMPORTANT — manually-copied mapper is a drift trap with no guard (Piotr). ✅ Fixed, and well done. The new "cross-plugin mapper parity" test block in allegro-category-catalog-client.spec.ts loads Allegro's own real sandbox fixture (category-parameters-257933.json) and asserts the same section/dependsOn behavior Allegro's own mapper spec asserts, with explicit cross-referencing doc comments added in both spec files and in the mapper function's own header. This doesn't eliminate the duplication (ADR-030 forbids the cross-plugin import that would), but it converts a silent-drift risk into a same-fixture, mirrored-assertion test pair — exactly what was asked for.

IMPORTANT (forward-looking, for #1383) — shared token cache once wired (Piotr). Correctly left alone in this PR — explicitly deferred to #1383 in the author's summary, and that's the right call since this sub-task is intentionally standalone with no adapter/factory wiring yet.

SUGGESTION — no single-flight guard on token acquisition (mine + Piotr, independently). ✅ Fixed. inFlightToken: Promise<CachedToken> | undefined added; ensureToken() shares the in-flight acquisition and clears it in a .finally(). New test (token single-flight) verifies exactly one /auth/oauth/token call for two concurrent fetchCategories + fetchCategoryParameters calls on a cold cache.

SUGGESTION — untested 401/403 branch on data calls, distinct from token-endpoint rejection (mine). ✅ Fixed. New describe('auth rejection on a data call...') block tests both /sale/categories (401) and /sale/categories/{id}/parameters (403) rejecting the bearer token.

SUGGESTION (edge case) — missing expires_in caches indefinitely (mine + Piotr, independently). ✅ Fixed correctly. acquireToken() now sets expiresAt: tokenData.expires_in ? Date.now() + tokenData.expires_in * 1000 : undefined, and ensureToken()'s cache-hit guard requires expiresAt !== undefined — so a token response without expires_in is now treated as always expired (forces re-acquisition on every call) rather than cached forever. New regression test confirms a second call re-acquires when expires_in is absent.

SUGGESTION — inline 'sandbox' | 'production' union (Piotr). ✅ Fixed. Replaced with AllegroCatalogEnvironmentValues = ['sandbox', 'production'] as const + derived AllegroCatalogEnvironment type in erli-connection.types.ts, matching AllegroConnectionConfig's own convention, per the as const pattern in engineering-standards.md.

SUGGESTION — ErliAuthenticationException reuse note (mine). Correctly left as a forward-looking note for #1383 (classifier wiring), no action needed here.

Documentation gap (mine, not code) — ADR-030 / implementation plan not yet on main or the epic base branch. Still true — PR #1380 (the plan+ADR PR) is still open. Not something this PR's author can or should fix; it's a merge-sequencing note for whoever merges the epic stack. Doesn't block this PR.

Fresh findings from this pass

None that block. Independently re-verified, with nothing new to add:

• ADR-030 boundary: confirmed again — no @openlinker/integrations-allegro import anywhere in the diff, only @openlinker/core/listings (type-only) + this package's own domain/exceptions/.
• File header, naming, layer placement (plain class in infrastructure/http/, kept out of the package barrel like ErliHttpClient) — all correct.
• No any types; unknown/typed generics used throughout; async/await used correctly; errors converted to typed exceptions with no silent swallowing.
• The parity-test fixture path resolves correctly (verified on disk) and the dependsOn mapping test (parameter 229205, "Stan opakowania") exercises a real multi-entry dependency union, not just the trivial case.

Merge readiness

🔄 Approve. Every IMPORTANT and SUGGESTION item from both prior review rounds has been genuinely addressed — not superficially, the fixes are correct on inspection (the expires_in fix in particular is subtle and was done right: cache-miss on undefined, not a truthy/falsy trap). Lint, type-check, and the full Erli test suite are green in a real worktree run (17/17 suites, 312/312 tests), and the cross-context import invariant is clean. The only open item is the ADR-030/plan doc PR (#1380) still being unmerged, which is a housekeeping note for the epic stack, not a defect in this PR. I'd consider this mergeable into epic/1381-erli-allegro-category-catalog as-is.

[norbert-kulus-blockydevs]

Thanks for the independent re-verification. This latest review confirms all IMPORTANT/SUGGESTION items from both prior rounds were correctly addressed in 306299a and raises no new blocking or non-blocking findings — the only remaining note (ADR-030/implementation plan not yet merged via #1380) is a housekeeping item for the epic stack, not something actionable in this PR. No further changes needed here.

[piotrswierzy]

/pr-review (re-review) — Erli-owned Allegro client_credentials category-catalog client

Re-reviewed at 306299a2 (the "address PR #1388 review findings" commit). Every item from my prior approve-with-changes is resolved — clearing to a full approve.

Findings → resolutions (all verified in the diff)

• 🟡 Mapper-drift guard → resolved. The Allegro allegro-category-parameter.mapper.spec.ts gained a "Parity note (#1382 review)" pointing at the Erli spec, which runs the same fixture through AllegroCategoryCatalogClient's copy of toNeutralCategoryParameter with mirrored assertions + reciprocal comments on both sides. Lighter than a single deep-equal parity assertion, but it's a real, documented cross-spec guard — a behavior change to either mapper now trips mirrored assertions and the comments name the sibling to update. Acceptable.
• 🟢 Single-flight on token acquisition → added. inFlightToken: Promise<CachedToken> is shared by concurrent ensureToken() callers and cleared in .finally() — concurrent parameter fetches now collapse to one client_credentials round-trip.
• 🟢 Indefinite cache when expires_in is absent → fixed. The cache-hit guard now requires this.cached.expiresAt !== undefined (client.ts:122-124); with expiresAt: undefined (:170) the token is re-acquired every call instead of being trusted forever. Correct fail-safe.
• 🟢 allegroEnvironment inline union → done. Now AllegroCatalogEnvironmentValues = ['sandbox','production'] as const + derived AllegroCatalogEnvironment (engineering-standards as const + runtime-array convention).
• Inline types → extracted to allegro-category-catalog-client.types.ts.

Still verified-clean

• ADR-030 plugin independence holds — no @openlinker/integrations-allegro import; the two grep matches are comments documenting the deliberate non-import. Imports are @openlinker/core/listings neutral types + own Erli*Exceptions + the local .types file.
• Typed exceptions, no any, additive/optional config fields.

Forward note (for #1383, not this PR)

Unchanged from before: the token cache is per-client-instance, so once #1383 wires this behind ErliAdapterFactory (which builds a fresh adapter per getCapabilityAdapter call), the cache lives one request. The single-flight + expiry-aware cache added here are the right primitive; #1383 should hoist the cache to a shared layer (module-scope keyed by clientId, or HostServices.cache) so it survives across adapter instances. Tracked for the wiring PR.

Final Verdict

✅ Approve — all review findings addressed; mergeable_state: clean. Ships cleanly onto the epic branch.