refactor(core): own provider retirement outside the metadata-built registry

[bytelazy]

Summary

isRetiredProvider lived in provider-registry.ts, which is built from the generated models.dev tables. The Desktop first screen imported it through command-palette-commands.ts, so the whole model-metadata snapshot stayed on the static startup path — the surviving chain flagged in the docs/model-metadata-firstscreen-optimization.md audit under #4711.

This extracts the predicate into a new metadata-free module, packages/core/src/provider-retirement.ts, backed by a small RETIRED_PROVIDER_TYPES list. Its only provider-registry import is import type (erased at build time), so importing it no longer pulls the generated tables. The startup-path importer (command-palette-commands.ts) and the two defaults.retired readers (model-catalog.ts, provider-auth.ts) are re-pointed at it; the retired flag is dropped from ProviderDefaults and the claude-subscription entry.

Refs #4711

Verification

• Static import-graph trace from apps/desktop/src/renderer/main.tsx (value imports only; import type erased; dynamic import() treated as a lazy boundary): the AppShellOverlays → … → command-palette-commands → provider-registry chain no longer reaches provider-registry — 0 chains after this change (was 1). Tracing from command-palette-commands.ts itself: 0 chains to provider-registry.
• model-metadata.generated.ts reachability from the renderer entry is unchanged by this PR: a pristine main worktree and this branch both report the same 2 chains (via core/settings → model-thinking → model-metadata, and via features/connection-settings → provider-add-model-dialog → llm-connections → provider-registry). This PR neither adds nor removes any of them.

  Correction: an earlier version of this line claimed "0 chains". That was a measurement artifact, not a fact: model-metadata.generated.ts is generated by scripts/sync-model-metadata.mjs and gitignored, so it is absent from a fresh checkout, and a disk-resolving import tracer silently reports zero chains to a file that isn't there. With the generated file present in both trees the counts match.

• Contract test provider-catalog-contract.test.ts now pins RETIRED_PROVIDER_TYPES as the source of truth and asserts isRetiredProvider agrees with the registry's retired set; a name that is retired but not registered (or vice versa) fails there rather than at send time.
• New unit test provider-retirement.test.ts (5 cases) covers the predicate's purity and the unknown/active/retired cases. Verified by compiling to a temp directory and running the emitted JavaScript (what test:dist does): compile exit 0, suite 5 pass / 0 fail.
• Import specifier: verified A/B with TypeScript 5.9.3 under the repo's moduleResolution: "Bundler" — a .ts specifier reproduces TS5097 (exit 2), the .js specifier is clean (exit 0). This was the P1 found in review and fixed in 69e0ea6.
• Swept for remaining defaults.retired / retired: true literals on ProviderDefaults — none left.

Correction to an earlier version of this description: it claimed the unit suite ran "5 pass, 0 fail" under node --test --experimental-strip-types. That passed only because the test imported ../provider-retirement.ts; that command resolves .ts directly and never exercises the compiler path build/typecheck use, so it masked a build break (TS5097) that review caught. With the correct .js specifier that command fails with ERR_MODULE_NOT_FOUND here, because the repo's suites run against compiled dist/. The emitted-JavaScript run above replaces that claim.

Checks I did not run: the repo's own npm --workspace @maka/core run build / typecheck / test:dist, lint, format, or the full suite. This environment cannot run npm ci (network-limited), so verification used a standalone TypeScript 5.9.3 against the repo's compiler options plus static import-graph analysis — not byte-level bundle evidence. CI shows action_required (first-time fork PRs need a maintainer to approve the workflow run), so no CI check has validated this branch yet.

A separate static chain still reaches provider-registry from the first screen (ui → maka-uri → settings → onboarding → llm-connections → provider-registry). This is intentionally not closed here and is recorded as open in the doc's audit note — it is the buildChatModelChoices/modelMenuGroups seam the doc's "Desired outcome" section already names, not the chain this change targets.

AI use

• No generative tool made a substantive contribution
• Generative tooling made a substantive contribution

Tool(s) and scope: Claude (claude-opus-4-8) authored the refactor, tests, and PR description. Both commits carry a Generated-by: Claude (claude-opus-4-8) trailer.

Checklist

• Tests cover the change and fail without it
• Lint, format, typecheck and the affected suites pass locally

Does this PR entail a change in behavior?

• Yes — described under Summary above
• No

The ProviderDefaults type loses its retired?: true field. Runtime behavior is unchanged: isRetiredProvider returns the same answers (it still resolves claude-subscription as retired), and the two readers that checked defaults.retired now call the predicate. No external API beyond that type field changes.

[me2seeks]

Automated review (Command Code) — not an approval

The extraction itself is sound: I checked that providerDefaultsOf(input.providerType) is the same input in both rewritten call sites (provider-auth.ts:59, model-catalog.ts:198), so defaults.retired === true → isRetiredProvider(providerType) is behavior-preserving, and the registry re-export keeps existing importers working. One problem blocks the build.

P1 (Must-Fix) — the new test file does not compile; @maka/core typecheck and build fail on this revision.

packages/core/src/__tests__/provider-retirement.test.ts:25 imports with a .ts extension:

} from '../provider-retirement.ts';

The repository's compiler options do not allow that. tsconfig.base.json sets "moduleResolution": "Bundler" and does not enable allowImportingTsExtensions — which in any case requires noEmit or emitDeclarationOnly, so it cannot be turned on for a package that emits to dist/. Every other import in the tree uses the emitted .js specifier (there are zero .ts-extension imports on main).

Evidence, running the package's own configuration on the reviewed revision:

$ npx tsc -p packages/core/tsconfig.json --noEmit
packages/core/src/__tests__/provider-retirement.test.ts(25,8): error TS5097: An import path can only end with a '.ts' extension when 'allowImportingTsExtensions' is enabled.

That is the only error reported, so the rest of the package is clean. npm --workspace @maka/core run build and run typecheck therefore fail, and because test:dist runs the compiled output, the new suite cannot run. No CI check has reported on this branch, so nothing else caught it.

Smallest sound fix: use the emitted specifier, from '../provider-retirement.js'. Verified locally — with that single change npx tsc -p packages/core/tsconfig.json --noEmit exits 0.

Review-relevant risks. None identified beyond the build break: no public contract, wire shape, security boundary, dependency, licensing, or release effect was found in the diff. The new RETIRED_PROVIDER_TYPES ownership and the contract test that pins it against the registry are an improvement over the implicit retired flag.

Required conclusion.

1. Optimal for the actual problem? Yes — the predicate moves to a metadata-free module and the duplicated retired flag is removed; that is the smallest change that cuts the static chain.
2. Production code that can be deleted? Done in this PR — the retired field and the registry-local predicate are gone, and nothing else still reads .retired.
3. Low-quality tests to delete or replace? none identified. The new suite is small but meaningful, and the contract test now ties the retired list to the registry.
4. Deeper refactor required? No.
5. Ready to merge? No — not until the import specifier is fixed; the package does not compile as pushed.
6. Residual risks / verification gaps: the doc's audit note states a second static chain remains (main.tsx → @maka/ui → maka-uri → core/settings → … → provider-registry); that is recorded, not fixed, and is out of scope here. I did not run the full suite; the typecheck above was run directly.

Approval boundary. This is automated review; it is not an approval. Per CONTRIBUTING.md, the merge decision requires an independent human review. No approve was submitted.

[bytelazy]

Thanks — P1 confirmed and fixed in 69e0ea6.

You're right on every point, and the finding also exposes a bad verification claim in my PR description, which I want to correct explicitly rather than quietly fix.

The bug. provider-retirement.test.ts:25 imported ../provider-retirement.ts. I reproduced TS5097 under the repo's resolution mode and confirmed the fix, A/B with TypeScript 5.9.3:

• .ts specifier → error TS5097: An import path can only end with a '.ts' extension when 'allowImportingTsExtensions' is enabled. (exit 2)
• .js specifier → clean (exit 0)

I also confirmed there are zero other .ts-extension imports in the tree, so this was mine alone, and that allowImportingTsExtensions is not a viable escape here since it requires noEmit/emitDeclarationOnly and @maka/core emits to dist/. Fixed to ../provider-retirement.js.

Correcting my own verification claim. My PR description said the unit test ran "5 pass, 0 fail". That was true only because of the wrong specifier: I ran node --test --experimental-strip-types, which resolves .ts directly and never invokes the compiler path build/typecheck use. With the correct .js specifier that same command actually fails with ERR_MODULE_NOT_FOUND, because the repo's suites run against compiled dist/ and I have no build in this environment. So my green signal was masking the break you found — the specifier error was load-bearing for my own evidence. That is a real gap in how I verified, not just a typo.

To verify properly this time I compiled both files to a temp directory and ran the emitted JavaScript, which is what test:dist does: compile exits 0, suite passes 5/5.

Still not run by me: the repo's own npm --workspace @maka/core run build / typecheck / test:dist, plus lint and format. This environment cannot npm ci (network-limited), so I verified with a standalone TypeScript 5.9.3 against the repo's compiler options rather than the workspace scripts. No CI has reported on this branch yet — it shows action_required, since first-time fork PRs need a maintainer to approve the workflow run. Worth having that run before anyone relies on my numbers.

The second static chain you noted (main.tsx → @maka/ui → maka-uri → core/settings → … → provider-registry) remains out of scope here and is recorded as open in the doc's audit note, as you read it.

[me2seeks]

Follow-up (Command Code) — verified resolved

I re-checked the head after your push.

The P1 build break is fixed. packages/core/src/__tests__/provider-retirement.test.ts now imports '../provider-retirement.js', which is the emitted specifier the package config requires. That was the only error npx tsc -p packages/core/tsconfig.json --noEmit reported, so the package typechecks again and the new suite can run. Nothing else in my review is outstanding.

Thanks for the quick turnaround. (Automated review; not an approval, and not a substitute for independent human review under CONTRIBUTING.md.)

[bytelazy]

Since that follow-up main moved and this branch went to dirty, so I rebased onto f109ccde. Force-pushed as 2e8dc112; mergeable_state is back to blocked (approval only). Two conflicts, both from #5225 (refactor(models): unify connection-scoped model configuration) touching the same hunks:

• provider-registry.ts — upstream removed relayModelProfiles from ProviderDefaults. Kept that removal, applied my change (drop retired, leave the ownership comment).
• model-catalog.ts — upstream reworked the call (relayModelProfiles → modelOverrides, plus a new savedModelIds derivation). Kept upstream's logic verbatim, changed only defaults.retired === true → isRetiredProvider(connection.providerType).

Re-verified after the rebase: typecheck of both files clean (exit 0); compiled to a temp dir and ran the emitted JavaScript, 5/5. Swept again for defaults.retired / retired: true on ProviderDefaults — none left. Both commits keep their Generated-by trailer.

I have also corrected my PR description. It claimed model-metadata.generated.ts is unreachable from the renderer entry (0 chains) and that this PR leaves that unchanged. The second half is right; 0 chains was wrong. model-metadata.generated.ts is produced by scripts/sync-model-metadata.mjs and gitignored, so a fresh checkout does not have it, and my tracer resolves specifiers against files on disk — in a tree where that file is absent it reports zero chains. With the generated file copied into a pristine main worktree, main reports the same 2 chains my branch does:

main.tsx → @maka/ui → maka-uri → core/settings → core/model-thinking → core/model-metadata → model-metadata.generated
main.tsx → composition/desktop-feature-services → features/connection-settings → provider-add-model-dialog → core/llm-connections → core/provider-registry → model-metadata.generated

So the pre-existing reachability is broader than the doc's audit note records: the second chain runs through connection-settings, which the note does not mention. That is upstream state, not something this PR changes. I have fixed the description to say "unchanged by this PR" without the false zero, and left the doc note alone rather than widen its scope inside a refactor PR.

The command-palette-commands → provider-registry claim this PR is about does hold after the rebase: 0 chains, was 1. It does not depend on the generated file, since it terminates at provider-registry.ts, which is committed.

Still not run by me: the repo's own build / typecheck / test:dist, lint, format (this environment cannot npm ci). CI still shows action_required, pending a maintainer approving the run for a first-time fork PR.

[hqhq1025]

Reviewed current head 966f4a57e039a64c7481c6defea21fc8001b2321 (12 files, +220/−23). Retirement ownership moves from flags in the metadata-built provider registry to a small type-only-dependent module (packages/core/src/provider-retirement.ts:46-65), with the old registry export retained for existing callers (packages/core/src/provider-registry.ts:1734). I traced the updated readiness, catalog, auth, connection-test, and Desktop command-palette call sites, plus the core/runtime regression tests. The three retired provider IDs remain registered but use unavailable adapters; the Runtime test path refuses them before making a request (packages/runtime/src/test-connection.ts:161-167). I found no substantiated P0–P3 code issue in these paths.

This branch is not ready to merge: there are no checks reported for this head, and a fresh synthetic merge with current main conflicts in packages/core/src/provider-registry.ts. Please resolve the conflict, run the required checks, and validate the resulting head. I did not run local build/tests (Node 18/no installed dependencies) or a real Desktop startup trace; the documentation itself notes another static metadata import path remains. No database schema/migration changes were made. This is not a merge approval.

Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.

[hqhq1025]

The rebase and follow-ups keep the retired-provider decision in the lightweight module while restoring the runtime connection-test and storage-import guards. I found no substantiated P0–P3 issue in the 14-file PR diff. The core/storage/runtime builds and four focused test files pass locally (30/30); the generated retirement module has no runtime imports, and the branch merges cleanly with current main. There are no hosted checks on this head, and I did not run the full suite or a packaged Desktop startup, so those gates remain unverified.

Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.

[me2seeks]

Automated review notice: This comment was posted by an automated review agent operated by me2seeks make. It is not an independent human review and does not replace one.

Summary

Owns provider retirement outside the metadata-built registry: a new provider-retirement.ts becomes the single authority for isRetiredProvider, and model-catalog.ts, provider-auth.ts, and connection-readiness.ts now consult it instead of defaults.retired === true; provider-registry.ts drops its retired?: true field with a comment documenting the move. This is precisely the seam the first-screen audit (#4711, already reviewed) named as the minimal break in the app-shell → provider-registry import chain — the small retirement predicate no longer requires the generated models.dev tables. The first-screen optimization doc is updated in step. audit passes.

Findings

1. [P1] test fails on this head — the required merge gate is red. For a retirement-authority extraction whose whole purpose is enabling the first-screen optimization, the provider-catalog-contract or provider-retirement suite is the likely failure; pull the log, fix, and show green before merge.
2. [P3] command-palette-commands.ts is touched — confirm it now imports isRetiredProvider from provider-retirement (not provider-registry), which is the actual chain-break; the audit doc in this PR claims it.

Verdict

needs-changes — the right extraction (and the one the audit called for), but the red test gate must be fixed first.

[hqhq1025]

The new commit routes the command palette's isRetiredProvider import through a narrow Desktop application contract (apps/desktop/src/renderer/command-palette-commands.ts:49, apps/desktop/src/renderer/application/contracts/provider-retirement.ts:20-22) and updates the architecture ledger and first-screen audit. The re-export still targets the metadata-free @maka/core/provider-retirement module, so I found no new P0–P3 issue in this increment or the inspected retirement boundary. This is a scoped review, not merge approval.

On this head, the renderer architecture check passed (112 tests and ledger check); the core build passed. A full local renderer build could not complete: after building core and runtime-host, the installed UI package failed on an unchanged SideNavItemProps.trailingAction type mismatch at packages/ui/src/session-history-list.tsx:782, leaving @maka/ui/composer-attachments unresolved for Vite. I did not run a packaged Desktop or first-screen timing measurement. The diff check and merge-tree against current main 03237142 are clean. No hosted checks are reported for this head, so its CI gate remains unverified.

Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.

[bytelazy]

Rebased onto main 3597abe8 and pushed 16b6d076. The new tests detect a reintroduced registry dependency and a missing early retirement guard.

Local checks pass: core/storage/runtime builds and typechecks; 920 core tests, 10 config-import tests, 17 runtime tests; the strict renderer architecture check (165 tests), lint and formatting. I did not rerun the full Desktop build.

The lockfile now matches main, including its ip-address/undici upgrades. Main's latest audit still fails on GHSA-6qxp-vccf-f47h in @modelcontextprotocol/client@2.1.0; this PR does not change that dependency.

Could a committer review this and approve the new workflow runs if required?

Automated update via Claude on behalf of the contributor.

[Astro-Han]

Automated review notice: This comment was posted by an automated review agent. It is not an independent human review and does not replace one.

Incremental review: 95dc93fa (our last reviewed head) to 16b6d076.

• The branch was rebased from 0fd75408 onto 3597abe8. git range-diff shows the five previously reviewed commits replayed with identical patches.
• There is one new commit, 16b6d076 ("test: enforce provider retirement dependency and guard boundaries"), which changes only tests. The PR is now 16 files, +291/-25.

New tests: sound.

• packages/core/src/__tests__/provider-retirement.test.ts:48-74 replaces the old assertion, which could not detect a dependency, with a check that has teeth:
  • It spawns a fresh Node process.
  • It registers a module.registerHooks resolve hook that throws on any specifier imported directly by the emitted provider-retirement.js.
  • It then imports that module.
• This is valid for this repo. Core tests run from dist (test:dist), so the .js URL exists and type-only imports are already erased. registerHooks is available on the engine floor (Node >=22.19) and on CI's Node 24, and runtime-host tests already use the same pattern.
• packages/runtime/src/__tests__/provider-retirement.test.ts now covers retired connections with no saved or explicit model as well. This exercises real ordering: testConnection returns through isRetiredProvider (test-connection.ts:165) before resolveConnectionTestModel (:171). Removing the early guard would therefore make the no-model case fail differently instead of passing silently.

Main interaction: none of the PR's files changed on main since 3597abe8 (#5902 is unrelated). git merge-tree against main 72afbf11 is clean.

Prior findings: none were open. The 966f4a5 conflict was resolved earlier, and 95dc93f found no P0-P3 issue.

Status: MERGEABLE, with merge state BLOCKED. Hosted workflows for this head (CI, Dependency audit) are in action_required, waiting for maintainer approval to run. CI is therefore unverified, and I did not run tests locally.

Verdict: no P0-P3 findings in this increment. This review is not merge approval; it needs CI to run and pass.