Skip to content

refactor(client-mapping): single agent-host registry#385

Merged
christso merged 1 commit into
mainfrom
refactor/377-host-registry
May 12, 2026
Merged

refactor(client-mapping): single agent-host registry#385
christso merged 1 commit into
mainfrom
refactor/377-host-registry

Conversation

@christso
Copy link
Copy Markdown
Contributor

Summary

Folds CLIENT_MAPPINGS and USER_CLIENT_MAPPINGS into a single AGENT_HOSTS registry — one entry per host, with paired project / user mappings. The legacy records are kept as derived constants (via Object.fromEntries(AGENT_HOSTS.map(...))) so existing consumers keep working unchanged, and future drift between project and user maps is impossible by construction.

Pattern modelled after cli/cli's internal/skills/registry/registry.go::AgentHost.

  • New helpers: findHostById, getMapping(id, scope), uniqueProjectSkillsPaths, agentHelpList.
  • Compile-time _HostsCoverEveryClient assertion + a runtime test walking every ClientTypeSchema value catches the "added to enum but forgot to register" bug class.
  • Intentional deltas (windsurf and copilot diverging between scopes) preserved and tested explicitly so future reviewers see them as deliberate.

Test plan

  • bun run build, bun run typecheck
  • bun test — 1197 pass / 0 fail
  • Verification gate from refactor: consolidate CLIENT_MAPPINGS + USER_CLIENT_MAPPINGS into a single agent-host registry #377:
    • Every ClientType value has exactly one AGENT_HOSTS entry (no duplicates).
    • getMapping(id, scope) returns identical content to CLIENT_MAPPINGS[id] / USER_CLIENT_MAPPINGS[id] for every (client, scope) pair.
    • The windsurf project/user delta is preserved (.windsurf/skills/ vs .codeium/windsurf/skills/).
    • All pre-existing client-mapping unit tests (~50) still pass.

Optional follow-up: migrate call sites from the legacy records to getMapping(id, scope) and drop the derived constants entirely.

Closes #377

@cloudflare-workers-and-pages
Copy link
Copy Markdown

cloudflare-workers-and-pages Bot commented May 12, 2026

Deploying allagents with  Cloudflare Pages  Cloudflare Pages

Latest commit: cc0e3f7
Status: ✅  Deploy successful!
Preview URL: https://b058bb6e.allagents.pages.dev
Branch Preview URL: https://refactor-377-host-registry.allagents.pages.dev

View logs

@christso
Copy link
Copy Markdown
Contributor Author

Code Review: ✅ APPROVE

This is a clean, well-scoped refactor. The new AGENT_HOSTS array is the single source of truth, legacy CLIENT_MAPPINGS / USER_CLIENT_MAPPINGS are derived (and Object.freeze-d) so they can't drift, and the compile-time _HostsCoverEveryClient assertion catches the "added to enum but not registered" bug at TS check time. Pattern matches the issue's reference (cli/cli registry.go).

Verification gate (#377)

  • (1) Every ClientType value covered exactly once — both compile-time (_HostsCoverEveryClient) and runtime (AGENT_HOSTS covers every ClientType exactly once) ✓
  • (2) Existing tests pass (1197 / 0) ✓
  • (3) getMapping matches both legacy records for every (client, scope) pair (asserted in a single loop test) ✓
  • (4) Windsurf and Copilot project/user deltas preserved with explicit tests ✓

CLAUDE.md compliance

  • Conventional commit (refactor(client-mapping): ...) ✓
  • No Co-Authored-By
  • "One test per distinct code path" — each new helper (findHostById, getMapping, uniqueProjectSkillsPaths) gets coverage; equivalence with legacy maps gets one combined loop test rather than 23 individual tests ✓
  • No premature abstraction; helpers are scoped to the consolidation task ✓

Design notes worth calling out (positive)

  • getMapping throws on missing host, findHostById returns undefined. The doc comment explains why: returning undefined from the canonical resolver would silently mask the bug class this refactor exists to eliminate. Good call.
  • Frozen legacy records. Object.freeze on CLIENT_MAPPINGS and USER_CLIENT_MAPPINGS prevents call sites from mutating what's now a derived view.
  • Intentional deltas are explicit comments. Windsurf's .codeium/windsurf/ and Copilot's .copilot/ user-scope paths have inline comments saying why. Future reviewers won't have to guess whether the divergence is a bug.

Minor (non-blocking)

1. Dead runtime use of ClientTypeSchema in client-mapping.ts.

The new import { ClientTypeSchema, type ClientType } brings in the runtime value, but it's only used as void ClientTypeSchema; at the bottom. The compile-time _HostsCoverEveryClient only needs the type ClientType (already imported as type). The tests use ClientTypeSchema from their own import path. Dropping the runtime import and the trailing void would be cleaner:

import type { ClientType } from './workspace-config.js';
// ... no `void ClientTypeSchema;` needed

2. agentHelpList() has no current call site.

The helper is implemented and tested elsewhere but isn't referenced anywhere in this PR. The issue mentions it'll be wired into the singular-rename's help text. Fine to land prep-work, but worth a one-line PR comment noting the intended consumer or a TODO that points at the issue, so it doesn't sit as dead code if the follow-up gets dropped.

3. The runtime _check indirection.

const _check: _HostsCoverEveryClient = true;
void _check;

Same compile-time guarantee can be expressed at the AGENT_HOSTS declaration with satisfies:

} as const satisfies readonly AgentHost[];

Plus a separate type _Cover = ... line. Cosmetic; the current form works.


Optional follow-up

PR body notes a follow-up to "migrate call sites from the legacy records to getMapping(id, scope) and drop the derived constants entirely." Worth filing now as an issue so the cleanup doesn't get lost.

Approving.

`CLIENT_MAPPINGS` and `USER_CLIENT_MAPPINGS` were two near-duplicate
~130-line records that had already drifted (windsurf's user-scope path
differs from its project-scope path, but the divergence was hard to spot in
review). Consolidates both into a single `AGENT_HOSTS` array — one entry per
host, with paired `project` and `user` mappings.

- `AgentHost` interface and `AGENT_HOSTS` const list every supported client
  exactly once. Pattern modelled after `cli/cli`'s
  `internal/skills/registry/registry.go::AgentHost`.
- `CLIENT_MAPPINGS` / `USER_CLIENT_MAPPINGS` are now derived via
  `Object.fromEntries(AGENT_HOSTS.map(...))` so future drift between project
  and user maps is impossible by construction.
- New helpers: `findHostById`, `getMapping(id, scope)`,
  `uniqueProjectSkillsPaths`, `agentHelpList`.
- Compile-time `_HostsCoverEveryClient` assertion + a runtime test that
  walks every `ClientTypeSchema` value catches the
  "added to enum but forgot to register" bug class.
- Intentional deltas (`windsurf` and `copilot` differing between scopes)
  preserved and tested explicitly so future reviewers see them as deliberate.

All existing call sites (`CLIENT_MAPPINGS.claude`, etc.) keep working — the
exports are the same shape, just frozen and derived. Optional follow-up:
migrate consumers to `getMapping(id, scope)` and drop the derived records.

Closes #377
@christso christso force-pushed the refactor/377-host-registry branch from a61bd5c to cc0e3f7 Compare May 12, 2026 14:14
@christso christso merged commit 446c681 into main May 12, 2026
1 check passed
@christso christso deleted the refactor/377-host-registry branch May 12, 2026 14:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

refactor: consolidate CLIENT_MAPPINGS + USER_CLIENT_MAPPINGS into a single agent-host registry

1 participant