docs(plan): Erli category/parameter browsing via Erli-owned Allegro client-credentials token

[norbert-kulus-blockydevs]

Summary

• Implementation plan for letting the Erli offer wizard browse Allegro's public category/parameter catalog (grant_type=client_credentials) without requiring the operator to own a real Allegro seller connection.
• ADR-030 documents the decision (Erli-owned client-credentials client, per-instance dynamic capability wiring) and the rejected alternatives (require a real Allegro connection, cross-plugin package dependency, shared system-wide credential).
• Design mockup (approved) is shared in the tracking epic issue's artifact link.

Plan

See docs/plans/implementation-plan-erli-allegro-category-catalog.md — 3 sequential sub-tasks:

1. Backend: Erli-owned Allegro client-credentials catalog client
2. Backend: Erli connection credentials + adapter wiring + validators
3. Frontend: connection wizard checkbox + offer wizard category/parameters steps

Test plan

• Epic + 3 child issues created via /create-issue referencing this plan
• Each sub-task implemented via /work, merged sequentially onto one integration branch
• Final PR (full functionality) opened to main for review

🤖 Generated with Claude Code

[piotrswierzy]

Code Review: Erli category/parameter browsing via Erli-owned Allegro client-credentials (docs/plan + ADR)

Scope: docs-only — docs/architecture/adrs/030-erli-allegro-category-catalog-via-client-credentials.md (new), docs/plans/implementation-plan-erli-allegro-category-catalog.md (new, +317), docs/architecture/adrs/README.md (+1 row). No code.

1️⃣ Summary

A well-reasoned ADR + an unusually rigorous implementation plan for letting the Erli offer wizard browse Allegro's public category/parameter catalog via a grant_type=client_credentials app token that Erli owns, without forcing an Erli-only operator to hold a real Allegro seller connection. The architecture is sound and consistent with the existing capability model: Erli becomes a per-instance conditional implementer of the existing CategoryBrowser/CategoryParametersReader ports (wired only when app credentials are configured), so the structural is* guards and connection.supportedCapabilities flip correctly — reusing the generic resolution path (CategoriesCacheService, the /listings/.../parameters endpoint) with no new endpoints and no CORE change. It correctly avoids a cross-plugin package dependency (ADR-003) and is designed specifically to not reintroduce the #1367 bulk-wizard regression. The single thing to resolve before merge is an ADR-number collision.

I verified every seam the plan names against main: the two capability files exist; isCategoryBrowser/isCategoryParametersReader are structural (typeof adapter.fetchCategories === 'function' / …fetchCategoryParameters); CategoriesCacheService.getAllegroCategories(connectionId, parentId?) exists; ErliAdapterFactory / credentials-validator / offer-manager-adapter all exist. The plan's file paths and capability names are accurate, not aspirational.

3️⃣ Issues & Risks

🔴 Blocking — ADR-030 number collision with open PR #1307

main tops out at ADR-029. This PR adds 030-erli-allegro-category-catalog-via-client-credentials.md and a README row for ADR-030 — but the still-open PR #1307 also adds 030-infakt-ksef-indirection.md and its own ADR-030 README row. Two open PRs both claim ADR-030. Whichever merges second will (a) duplicate the ADR number (the ADR README numbering convention requires unique sequential numbers), and (b) hit a merge conflict / duplicate row on README.md's table. This is the same coordination hazard as the migration-timestamp collisions.
Fix: renumber one to ADR-031 — filename, the in-file # ADR-0xx: heading + Status line, the README table row, and every cross-reference (the plan links to 030-erli-allegro-… in ~5 places). Since #1307 reserved 030 first, the low-friction move is for this PR to take 031 — or, whichever merges second renumbers. (Note: #1307 is a draft; if it stalls, this PR can keep 030 and #1307 renumbers — but the two must be deconflicted before both land.)

🟢 Optional

• allegroClientId stored inside the encrypted credentialsRef blob alongside the secret. The client_id is a public app identifier, not a secret — but the plan explicitly justifies keeping both in one credentials shape (the FE gate is supportedCapabilities, not a raw config field, so client_id needs no FE-visible surface), and cites the rejected config/credentials split. Defensible for v1; noting only so it's a conscious call.
• Revoked-credentials-mid-session edge case (token request throws after save-time success → exception propagates through CategoriesCacheService/listings.controller.ts as an unhandled error, same as any "adapter implements the capability but the call fails" today). The plan honestly documents this as a pre-existing generic gap and defers it. Fine — just ensure the eventual implementation logs it (not a silent 500) so operators can diagnose a rotated Allegro app.

2️⃣ Alignment with documentation — strong

• ADR template: follows the practice (Status/Date/Authors, Context, Decision, Alternatives considered — four, each with a rejection rationale, Consequences with pros/cons/migration, References). README index updated. ✅
• Plugin independence (ADR-003): no @openlinker/integrations-erli → @openlinker/integrations-allegro dependency; the token/fetch logic is re-implemented Erli-side (~100-150 LOC dup, explicitly acknowledged as the trade-off vs. coupling). ✅
• Capability model (ADR-002): conditional per-instance implementation of existing ports via structural guards — matches the documented "adapters declare capabilities they support" pattern, and the per-instance (not per-class) choice is precisely what prevents the #1367 regression for unconfigured connections. ✅
• No migration: additive fields inside the existing encrypted credentialsRef JSON + ErliConnectionConfig — consistent with how apiKey works today. ✅
• Consistency with ADR-023/025 borrowed-taxonomy: extends, doesn't contradict, the Erli-borrows-Allegro-taxonomy vocabulary. ✅

6️⃣ Plan quality

Execution-ready: 3 sequential sub-tasks (client → wiring/validators → FE), each step tied to a real file path with its own acceptance criterion, plus §5 Questions/Assumptions (both-or-neither validation, allegroEnvironment enum, edit-only-for-v1), §8 risks (with the #1367 re-run as an explicit mitigation), and §9 testing strategy (unit + the #991 HTTP-seam-fake integration test). This is above the bar for a plan.

Final Verdict

🔄 Approve with changes — the content is excellent and technically accurate; renumber the ADR to clear the 030 collision with #1307 (README row + heading + all cross-links) before merge. The two optional notes are for the implementing PRs, not this doc.

Priority fixes

1. Deconflict ADR number: renumber this ADR to 031 (or coordinate so whichever merges second renumbers), updating filename, in-file heading/status, README table row, and the plan's ~5 cross-reference links.

[piotrswierzy]

/pr-review — deep follow-up (supersedes my earlier approve-with-changes)

I went back into the actual dispatch code to stress-test the plan's core mechanism against the live seams it names, and found a blocking design gap that my first pass (scoped to the ADR-number collision) didn't dig into. Upgrading the verdict to request-changes on the plan, not the prose.

The backend fetch half is genuinely sound: Erli conditionally wiring fetchCategories/fetchCategoryParameters onto the adapter instance, resolved via getCapabilityAdapter('OfferManager') + isCategoryBrowser/isCategoryParametersReader narrowing, works per-connection. The problem is the signal the FE gates on.

🔴 BLOCKING — connection.supportedCapabilities is manifest-static; the per-connection gate the plan hangs the feature on can't work as described

connection.controller.ts:75 (toResponse): supported = metadata.supportedCapabilities, where metadata = resolveAdapterMetadata() → adapterRegistry.getAdapterMetadata(adapterKey) (integrations.service.ts:140) — the static manifest for the adapterKey. It never instantiates the adapter or probes the guards. Erli's manifest is supportedCapabilities: ['OfferManager', 'OrderSource'] (erli-plugin.ts:59). So:

• The wizard gate connection.supportedCapabilities.includes('CategoryBrowser') (plan §6 Phase 3, and the #1367 bulk gate) is false for every Erli connection, configured or not → the category/parameter steps never render → feature inert.
• "Fixing" that by adding CategoryBrowser/CategoryParametersReader to the Erli manifest makes the gate true for all Erli connections including unconfigured ones → the wizards show the parameter step (required "Stan") for connections whose instance has fetchCategories === undefined, so the category endpoint returns []/501 → exactly the #1367 regression ADR-030 § Decision claims its per-instance design prevents.

There is no static-manifest value that yields per-connection behavior, yet the plan explicitly forbids what would create one ("no new HTTP endpoints"; "no change to the bulk-wizard's existing gate"; connection.controller.ts listed under Existing Services Reused — no changes required). Required plan change: add a concrete per-connection capability-exposure mechanism — e.g. toResponse resolves the adapter instance and probes the guards to compute a dynamic capability list (async, N instantiations per connection-list render — weigh the cost); or a per-connection capability-probe endpoint; or gate the FE on a config-derived boolean (hasAllegroCatalogCreds) instead of supportedCapabilities. And rewrite ADR-030 § Decision — "flips to true only for configured connections via the existing gate" is the load-bearing claim and it's not achievable as written. Unit tests that mock supportedCapabilities would not catch this.

🟡 IMPORTANT — in-instance token cache is dead (adapters are built fresh per call), and the params path is uncached → a client_credentials token round-trip per parameter fetch

getAdapter's own comment (integrations.service.ts:78-82) + getCapabilityAdapter (:124, factoryResolver.createCapabilityAdapter(...)) confirm no memoization — a fresh adapter, and thus a fresh AllegroCategoryCatalogClient, is constructed on every call. So "in-memory token caching inside the client instance is sufficient" (§4, Phase 1 Step 3) holds for exactly one request. Combined with the plan's own note that the /parameters endpoint is uncached at the controller layer, every fetchCategoryParameters (fired per category selection) triggers a fresh token acquisition plus the fetch — 2 Allegro calls each, against a per-app rate budget. Allegro's own adapter avoids this via a shared token (AllegroConnectionTokenState/AllegroTokenRefreshService) and the global allegro:cat-params:{categoryId} cache (allegro-offer-manager.adapter.ts:112). Fix: cache the app token in a shared store keyed by clientId (the plan already has HostServices.cache and cites the Allegro global-cache pattern), and reconsider caching category parameters — this feature is what makes that gap hot on Erli.

🟡 IMPORTANT — the plan under-models the resolution gates

getCapabilityAdapter has two per-connection gates: metadata.supportedCapabilities.includes(cap) (:100) and connection.enabledCapabilities.includes(cap) (:108). The backend path resolves via 'OfferManager' (present + enabled-by-default), so the fetch isn't blocked — but §3's framing that "the existing generic resolution path already works for any connectionId whose adapter implements the capability" glosses over both gates and the OfferManager-indirection. Make it explicit so the implementer doesn't try to resolve 'CategoryBrowser' directly (which would hit the :100 gate and throw).

🟢 SUGGESTION — let the ADR own the real architecture gap it uncovers

Sub-capabilities in a static per-adapterKey manifest cannot express per-connection-conditional capabilities. #1370 got away with putting CategoryBrowser in Allegro's manifest only because it's uniform across all Allegro connections; Erli is the first per-connection-conditional case. The honest ADR framing is "the manifest capability model has no per-connection projection today — here's the one we're adding," rather than asserting the existing gate already handles it. That's the most valuable thing this plan surfaces.

Still open from my first pass

ADR-030 number collision with the open #1307 (030-infakt-ksef-indirection.md) — main tops at 029, both PRs claim 030. Renumber one (README row + heading + cross-links).

Verdict

❌ Request changes (to the plan) — the design is ~80% right and the backend per-instance dispatch is sound, but the single mechanism the feature hangs on (per-connection supportedCapabilities) doesn't exist, and the plan rules out the changes that would create it. Before /work: (1) add a concrete per-connection capability-exposure mechanism + rewrite the ADR § Decision claim; (2) resolve the token-cache/uncached-params interaction; (3) deconflict the ADR-030 number. None are hard — but shipped as written, sub-task 3's wizard gate would be inert or would regress #1367.

[piotrswierzy]

/pr-review (re-review) — Erli Allegro category-catalog plan + ADR

Re-reviewed after the update. Both items from my earlier Request-changes are resolved — clearing to approve, with one design-clarity item to settle before the wiring sub-task (#1383).

Resolved

• ✅ ADR-number collision → the ADR is now 031-erli-allegro-category-catalog-via-client-credentials.md, no longer colliding with #1307's 030-infakt-ksef-indirection.md. Clean.
• ✅ Static-manifest capability flaw → the plan now explicitly mandates per-instance runtime capability wiring (Step 2.1, line 147: "Do not add CategoryBrowser/CategoryParametersReader to the class's static implements clause… this must stay a per-instance, runtime-reflected capability"), with the constructor conditionally installing fetchCategories/fetchCategoryParameters only when Allegro creds are present, and line 178 confirming a misconfigured connection lands on the existing is*-guard-returns-false path (empty categories / 501 params). That's exactly the correction my deep review asked for.

🟡 One item to settle before #1383 implementation (not a plan blocker)

The plan still describes the FE gating on connection.supportedCapabilities.includes('CategoryBrowser') and says supportedCapabilities will be "correctly reflected" / "automatically start working for configured connections" (lines 33, 141). But connection.supportedCapabilities in the connection response is sourced from the static AdapterMetadata.supportedCapabilities (via resolveAdapterMetadata → toResponse), which is per-adapterKey, not per-connection — so it can't distinguish an Erli connection that configured Allegro creds from one that didn't. The per-instance adapter wiring (Step 2.1) is correct at the port layer, but the FE gate needs a genuinely per-connection signal. Before #1383, decide explicitly between:

1. surface a per-connection effective-capability list to the FE (e.g. derived from the connection's own enabledCapabilities + config, not the static manifest), or
2. accept that the manifest declares CategoryBrowser for all Erli connections and the step always renders, degrading to the plain-text fallback (already described on line 28) when the runtime guard returns false.

Either is defensible — but the plan's current "supportedCapabilities flips to true for configured connections" language reads as option (1) while relying on a static field, and that gap is exactly where this bit last time. Nail it down in #1383's step, not in a branch.

Verdict

✅ Approve — the hard blocker (ADR collision) is gone and the capability model is now correct at the adapter layer. The FE-gate-signal question above is a #1383 implementation decision to make consciously; flagging it so it doesn't resurface as a runtime surprise.