feat(types): centralize provider identifiers

[WebMad]

Summary

• add a lightweight canonical registry for active and retired provider identifiers
• derive the existing provider name collections and validation schemas from the registries
• preserve existing serialized values and public compatibility exports
• add focused tests for duplicates, active/retired separation, schema validation, type guards, and serialized-value compatibility

Scope

This foundational PR intentionally does not migrate provider category collections or production consumers.

Testing

• npx vitest run src/__tests__/provider-identifiers.test.ts src/__tests__/provider-settings.test.ts
• npm run check-types
• npm run lint
• npm test (236 tests passed)

Closes #951
Tracking issue: #944

Summary by CodeRabbit

• New Features

  • Added a centralized registry of active and retired provider identifiers.
  • Exposed provider identifiers and related types through the package’s public exports.
  • Preserved validation and type-guard support for active, retired, and unknown providers.

• Tests

  • Added coverage confirming identifier ordering, uniqueness, compatibility, separation, schema validation, and type-guard behavior.

[coderabbitai]

📝 Walkthrough

Walkthrough

Adds shared active and retired provider identifier registries with derived public types, re-exports them from packages/types, updates provider settings to derive validation values from those registries, and adds tests covering ordering, compatibility, schemas, and type guards.

Changes

Provider Identifier Registry

Layer / File(s)	Summary
Canonical registries and public exports packages/types/src/provider-identifiers.ts, packages/types/src/index.ts	Adds active and retired identifier maps, derived union types, and top-level package exports.
Provider settings integration and validation packages/types/src/provider-settings.ts, packages/types/src/__tests__/provider-identifiers.test.ts	Derives active and retired provider-name values from the registries and verifies compatibility exports, schemas, uniqueness, separation, and type guards.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Possibly related issues

• Zoo-Code-Org/Zoo-Code issue 944 — Proposes canonical provider identifier registries and derived validation, which this PR implements for the shared types while leaving other production literals unchanged.

Possibly related PRs

• Zoo-Code-Org/Zoo-Code#801 — Modifies the same provider settings naming and schema wiring.

Suggested reviewers: taltas

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly describes the main change: centralizing provider identifiers in types.
Description check	✅ Passed	The description covers the issue link, implementation summary, scope, and testing, with only noncritical template sections omitted.
Linked Issues check	✅ Passed	The PR introduces a shared identifier registry and keeps existing provider strings and public APIs unchanged, matching issue #951.
Out of Scope Changes check	✅ Passed	The added re-export, registry, and tests are all directly related to centralizing provider identifiers.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

🧹 Nitpick comments (1)

packages/types/src/provider-settings.ts (1)

119-119: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Restore compile-time immutability for the derived provider name arrays.

Previously, the hardcoded arrays were defined using as const, which made them readonly at compile time. Since Object.values returns a mutable array and the current tuple casts drop the readonly modifier, the derived arrays are now technically mutable at compile time. Consider casting these as readonly tuples to match the previous safety guarantees (Zod's .enum() natively accepts readonly tuples).

• packages/types/src/provider-settings.ts#L119-L119: add readonly to the providerNames tuple cast: as readonly [ProviderIdentifier, ...ProviderIdentifier[]].
• packages/types/src/provider-settings.ts#L132-L135: add readonly to the retiredProviderNames tuple cast.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/types/src/provider-settings.ts` at line 119, Restore compile-time
immutability for the derived provider name tuples by adding the readonly
modifier to the tuple casts in providerNames and retiredProviderNames. Update
both affected sites in packages/types/src/provider-settings.ts (lines 119-119
and 132-135); no other behavior should change.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@packages/types/src/provider-settings.ts`:
- Line 119: Restore compile-time immutability for the derived provider name
tuples by adding the readonly modifier to the tuple casts in providerNames and
retiredProviderNames. Update both affected sites in
packages/types/src/provider-settings.ts (lines 119-119 and 132-135); no other
behavior should change.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: bc71e3be-4647-4f05-9b1e-92df2eb2e702

📥 Commits

Reviewing files that changed from the base of the PR and between 6e05ae9 and ac840f1.

📒 Files selected for processing (4)

• packages/types/src/__tests__/provider-identifiers.test.ts
• packages/types/src/index.ts
• packages/types/src/provider-identifiers.ts
• packages/types/src/provider-settings.ts

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[zoomote]

Review: centralize provider identifiers

The core types change looks solid — the registry order in provider-identifiers.ts exactly preserves the previous providerNames ordering (dynamic → local → internal → custom → faux → static), serialized values are unchanged, and the new tests cover uniqueness, active/retired separation, schemas, and type guards well. Nice work on that part.

However, this PR contains two changes that are unrelated to its stated scope, one of which looks like a regression. Findings below with line anchors.

---

1. Race-condition regression — locking removed from cache invalidation
src/core/task-persistence/TaskHistoryStore.ts:432 (invalidate) and src/core/task-persistence/TaskHistoryStore.ts:448 (invalidateAll)

Both methods previously ran inside withLock(...), guaranteeing they couldn't interleave with an in-flight upsert/writeTaskFile. That guard is now gone:

• invalidate() can read a task file while an upsert for the same task is mid-write, get the stale contents, and repopulate the cache with stale data after the write lands.
• invalidateAll() can clear the cache mid-write and have the in-flight write re-add its entry afterwards, leaving the cache in exactly the state the caller tried to clear.

This isn't hypothetical — the two deleted tests in src/core/task-persistence/__tests__/TaskHistoryStore.spec.ts ("waits for an in-flight write before refreshing the cache" and "waits for an in-flight write before clearing the cache") were written specifically to pin this behavior. Deleting the lock and the tests that guarded it in the same PR, without any justification, reads as an accidental regression. If the lock removal is intentional (e.g. deadlock concern — note withLock is non-reentrant), it needs its own PR with an explanation and replacement coverage.

2. Out-of-scope workflow change
.github/workflows/code-qa.yml — removal of the "Upload coverage reports to GitHub" artifact step

Nothing in the PR description mentions CI coverage artifacts, and the PR explicitly describes itself as a foundational types-only change ("intentionally does not migrate provider category collections or production consumers"). If this upload step should go, it deserves its own PR so the intent and any downstream consumers of those artifacts can be evaluated separately. If it came along accidentally (branching off a dirty worktree?), it should be reverted here.

3. Misleading doc comment (nit)
src/core/task-persistence/TaskHistoryStore.ts:445-447

The new comment says "Clear all in-memory cache and reload from index", but the method only clears the cache — it no longer reloads anything (the caller in webviewMessageHandler.ts:966-968 follows up with reconcile() itself). The previous wording ("a subsequent reconcile() repopulates them") described this accurately; the new one overstates what the method does.

4. Nit — unchecked non-empty assertion
packages/types/src/provider-settings.ts:119 and :132

Object.values(providerIdentifiers) as [ProviderIdentifier, ...ProviderIdentifier[]] asserts a non-empty tuple for z.enum, but nothing enforces that at the type level — if the registry were ever emptied this cast would lie and z.enum would throw at module load. The runtime tests make this unlikely to slip through, so it's fine as-is; just noting that the safety now lives in tests rather than types.

---

Summary: the provider-identifier registry itself is good to ship. I'd ask for the TaskHistoryStore locking changes (and deleted tests) and the code-qa.yml change to be split out or reverted before merge — they're unrelated to #951 and the lock removal appears to reintroduce a real race.

(Note: posted as a single review comment rather than per-line inline threads — inline threading wasn't available from my tooling, hence the explicit file:line anchors.)