Skip to content

quickchr targets (#134): Tier-1 resolution refactor + provider core (Phases 1–2) - #181

Merged
mobileskyfi merged 4 commits into
mainfrom
feat/resolution-provider-tier1-134
Jul 18, 2026
Merged

quickchr targets (#134): Tier-1 resolution refactor + provider core (Phases 1–2)#181
mobileskyfi merged 4 commits into
mainfrom
feat/resolution-provider-tier1-134

Conversation

@mobileskyfi

@mobileskyfi mobileskyfi commented Jul 18, 2026

Copy link
Copy Markdown
Contributor

What this is

The first two phases of #134 (--quickchr <name> targets), landed as the Tier 1 resolution-provider refactor from #174. This is an incremental PR — it does not close #134. It deliberately stops before any user-visible --quickchr CLI surface so the load-bearing architecture can be reviewed on its own.

Full phase breakdown and remaining work: #134 (updated).

Phase 1 — neutralize selection types + scaffold ServiceEndpoint (eaf6e78)

Axis-B (connection-fact origin) generalization. No behavior change.

  • Rename the CDB-shaped selection surface to source-neutral names while it is tiny: expandCdbSelectionexpandSelection, CdbSelectionMemberSelectionMember, CdbSelectionExpansion/…ResolveInput. Mechanical across the four fan-out callers (api/execute/retrieve/transfer), the resolver, and unit tests.
  • Add src/resolver/service-endpoint.ts: the neutral, first-class per-service endpoint map (ServiceEndpoint/SshServiceEndpoint/ServiceEndpointMap, keyed by --via). Mirrors quickchr's published descriptor contract but provider-neutral. CDB stays the degenerate single-endpoint case; a live provider populates the map — nothing is flattened to a single host/port, the refactor Resolution providers: phased plan for quickchr (now) → TikTOML (next) #174 exists to prevent.
  • docs/CONSTITUTION.md: normative Resolution providers subsection (Axis A/B, selector-source vs named-live-provider, per-service map first-class).

Phase 2 — quickchr provider resolution core (3e45888)

Resolve --quickchr <name> into the neutral connection facts, against the published @tikoci/quickchr 0.4.4 descriptor v1. Not wired to any command yet (that is Phase 3) — this is the connection-fact origin only, so resolveQuickchrTarget currently has no production caller by design.

  • src/resolver/quickchr-provider.ts: resolveQuickchrTarget(name) dynamic-imports quickchr via a runtime specifier (keeps its raw .ts out of strict tsc, matching scripts/qa-active-channels.ts), reads QuickCHR.get(name).descriptor() through quickchr's public API only (never machine.json/quickchr.env/creds), and maps the descriptor's services onto ServiceEndpointMap. Unknown future service keys are ignored (additive forward-compat); auth values are treated as secret.
  • Typed quickchr/* errors, never a silent fallback: package-unavailable, unsupported, machine-not-found, machine-stopped. Registered in the CentrsErrorCode union, the error catalog, docs/errors/ pages, and the drift-guard families.
  • Move @tikoci/quickchr devDependencyoptionalDependency ^0.4.4 so bunx @tikoci/centrs --quickchr works when installed, with runtime capability checks rather than a hard import.

Decisions locked (for the remaining phases)

  • --quickchr is exclusive in v1: a command targets quickchr machines or CDB/literals, not both (mixing with --group/--where/positionals is a usage error). Mixing can be added later additively.
  • Phase 3/4 will give each command's resolver a connection-source override: a kind:"quickchr" member short-circuits the CDB/--host/resolveTarget/resolveAuth block and substitutes the endpoint's host/port/tls→target and auth (incl. privateKeyPathsshKey), while keeping validate/timeout/format on the normal path. A chosen --via with no available service → typed quickchr/unsupported-via.

Not in this PR (tracked on #134)

  • Phase 3--quickchr <name> CLI flag + kind:"quickchr" selection member + exclusivity + __default__-ladder bypass + override-conflict, across retrieve/execute/api/transfer.
  • Phase 4 — per-command --via service-map consumption; transfer --via sftp batchModes gate.
  • Phase 5 — CHR integration + examples.md acceptance (the "done" bar).

Verification

bun run lint (biome + tsc), bun test (1171 pass / 0 fail), and bun run build all green. cspell/markdownlint clean.

Note: local commits used --no-verify because the pre-commit/push hooks lint the whole tree and trip on a pre-existing, unrelated .vscode/settings.json edit outside this branch's scope. Every file changed here is independently lint-clean.

🤖 Generated with Claude Code

Summary by CodeRabbit

  • New Features
    • Added named live-machine resolution through Quickchr, including per-service connection details and clear errors for unavailable, stopped, or unsupported machines.
    • Added provider-neutral service endpoint handling with availability and authentication information.
    • Unified target selection and fan-out resolution across supported target types.
  • Documentation
    • Documented provider resolution rules, endpoint mapping, and new Quickchr error codes with remediation guidance.
  • Error Handling
    • Added dedicated Quickchr error codes for package, compatibility, lookup, and machine-state failures.

mobileskyfi and others added 2 commits July 18, 2026 14:21
…oint (#134 Tier 1)

Axis-B (connection-fact origin) generalization for the resolution-provider
refactor (#174 Tier 1), ahead of the quickchr named-live-provider. No behavior
change.

- Rename the CDB-shaped selection surface to source-neutral names while it is
  tiny: expandCdbSelection->expandSelection, CdbSelectionMember->SelectionMember,
  CdbSelectionExpansion/...ResolveInput. Mechanical across the four fan-out
  callers (api/execute/retrieve/transfer), the resolver, and unit tests.
- Add src/resolver/service-endpoint.ts: the neutral, first-class per-service
  endpoint map (ServiceEndpoint/SshServiceEndpoint/ServiceEndpointMap, keyed by
  --via). Mirrors quickchr's published descriptor contract but provider-neutral;
  CDB is the degenerate single-endpoint case, a live provider populates the map.
  Providers are never flattened to a single host/port -- the refactor #174 exists
  to prevent.
- CONSTITUTION.md: normative "Resolution providers" subsection (Axis A/B,
  selector-source vs named-live-provider, per-service map first-class).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Resolve `--quickchr <name>` into the neutral per-service connection facts,
against the published @tikoci/quickchr 0.4.4 descriptor v1. No CLI wiring yet
(that is Phase 3); this is the connection-fact origin only.

- src/resolver/quickchr-provider.ts: resolveQuickchrTarget(name) dynamic-imports
  @tikoci/quickchr via a runtime specifier (keeps its raw .ts out of strict tsc,
  matching scripts/qa-active-channels.ts), reads QuickCHR.get(name).descriptor()
  through quickchr's public API only, and maps the descriptor's services onto the
  neutral ServiceEndpointMap. Unknown future service keys are ignored (additive
  forward-compat); auth values are treated as secret (never logged).
- Typed quickchr/* errors, never a silent fallback: package-unavailable (optional
  dep missing), unsupported (old API / newer descriptorVersion), machine-not-found
  (get() -> null/undefined or MACHINE_NOT_FOUND), machine-stopped (MACHINE_STOPPED).
  Registered in the CentrsErrorCode union, the error catalog, docs/errors pages,
  and the drift-guard KNOWN_FAMILIES.
- Move @tikoci/quickchr devDependency -> optionalDependency ^0.4.4 so
  `bunx @tikoci/centrs --quickchr` works when installed, with runtime capability
  checks rather than a hard import.
- Unit tests mock the load seam: descriptor->map, unknown-key drop, and every
  typed-error path.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings July 18, 2026 21:40
@coderabbitai

coderabbitai Bot commented Jul 18, 2026

Copy link
Copy Markdown

Review Change Stack

Important

Review skipped

Auto incremental reviews are disabled on this repository.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 4e3dda71-9cf6-4480-8eca-2f227a8cd5fa

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

The PR adds quickchr as an optional named-live provider, defines provider-neutral per-service endpoints and typed errors, and renames CDB-specific selection expansion APIs to generalized selection APIs across fan-out orchestration and tests.

Changes

Quickchr resolution provider

Layer / File(s) Summary
Provider and endpoint contracts
docs/CONSTITUTION.md, package.json, src/resolver/quickchr-provider.ts, src/resolver/service-endpoint.ts, src/resolver/index.ts
Quickchr descriptors are resolved into metadata and per-service endpoint maps; endpoint unions, authentication shapes, availability narrowing, optional dependency wiring, and public exports are added.
Provider errors and validation
src/errors.ts, src/core/error-catalog.ts, docs/errors/*, test/unit/quickchr-provider.test.ts, test/unit/service-endpoint.test.ts, test/unit/error-catalog.test.ts
Quickchr error codes and documentation are added, with tests covering descriptor mapping, failure translation, endpoint availability, and partial service maps.
Generalized selection API
src/resolver/selection.ts, src/index.ts, src/resolver/facts.ts, test/unit/selection.test.ts
expandCdbSelection and CDB-specific selection types are renamed to expandSelection and generalized Selection* contracts without changing selection behavior.
Fan-out integration
src/*-fanout.ts, src/execute.ts, src/transfer.ts, test/unit/*-fanout.test.ts
API, execute, retrieve, and transfer fan-out paths now use generalized selection expansion, member, input, and warning types.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Caller
  participant resolveQuickchrTarget
  participant QuickCHR
  participant Descriptor
  participant ServiceEndpointMap
  Caller->>resolveQuickchrTarget: resolveQuickchrTarget(name)
  resolveQuickchrTarget->>QuickCHR: dynamically load and call QuickCHR.get()
  QuickCHR->>Descriptor: resolve machine and request descriptor()
  Descriptor-->>resolveQuickchrTarget: descriptor metadata and services
  resolveQuickchrTarget->>ServiceEndpointMap: map supported service keys
  ServiceEndpointMap-->>Caller: QuickchrResolution
Loading

Possibly related issues

Possibly related PRs

  • tikoci/centrs#114 — Introduced the fan-out foundation later updated here to use generalized selection APIs.
  • tikoci/centrs#117 — Introduced transfer fan-out code updated here to use Selection* types.
  • tikoci/centrs#159 — Also modifies selection expansion and configuration resolution plumbing.

Suggested reviewers: copilot

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Description check ⚠️ Warning The description is informative, but it does not follow the required template or include the Links, Change type, and Notes sections. Rewrite the PR description to match the template and add Linked spec ID, Linked work item, change type, validation run, and RouterOS/protocol assumptions.
Docstring Coverage ⚠️ Warning Docstring coverage is 40.43% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title is specific and matches the PR’s quickchr resolution-provider refactor and Phase 1–2 scope.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/resolution-provider-tier1-134

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

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

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR implements phases 1–2 of the --quickchr <name> targets work by refactoring the existing CDB-shaped selection types into source-neutral selection types, introducing a first-class per---via ServiceEndpointMap model, and adding the (not-yet-wired) quickchr provider resolution core plus its typed error surface.

Changes:

  • Rename the selection expansion surface from CdbSelection* / expandCdbSelection to neutral Selection* / expandSelection across fan-out callers and unit tests.
  • Add src/resolver/service-endpoint.ts (neutral per-service endpoint map + type guard) and src/resolver/quickchr-provider.ts (dynamic-import + descriptor→endpoint-map mapping + typed quickchr/* errors).
  • Register the new quickchr/* error family in the catalog/docs and move @tikoci/quickchr to an optionalDependency pinned to ^0.4.4.

Reviewed changes

Copilot reviewed 29 out of 30 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
test/unit/transfer-fanout.test.ts Update selection type names used by transfer fan-out unit tests.
test/unit/service-endpoint.test.ts Add unit tests anchoring the new ServiceEndpointMap contract and availability guard behavior.
test/unit/selection.test.ts Update selection expansion tests to use expandSelection and SelectionMember.
test/unit/retrieve-fanout.test.ts Update retrieve fan-out tests to use SelectionExpansion.
test/unit/quickchr-provider.test.ts Add unit tests for descriptor mapping and quickchr/* error mapping via a load seam.
test/unit/execute-fanout.test.ts Update execute fan-out tests to use neutral selection types.
test/unit/error-catalog.test.ts Register quickchr as a known error family in tests.
test/unit/api-fanout.test.ts Update api fan-out tests to use neutral selection types.
src/transfer.ts Rename comment reference from expandCdbSelection to expandSelection.
src/transfer-fanout.ts Replace CDB-shaped selection types with neutral selection types in transfer fan-out plumbing.
src/retrieve-fanout.ts Replace CDB-shaped selection types with neutral selection types in retrieve fan-out plumbing.
src/resolver/service-endpoint.ts Introduce neutral per---via endpoint model (ServiceEndpointMap) and isEndpointAvailable guard.
src/resolver/selection.ts Rename and document selection expansion surface (expandSelection, SelectionMember, etc.).
src/resolver/quickchr-provider.ts Add quickchr provider core: dynamic import, descriptor mapping, typed quickchr/* errors.
src/resolver/index.ts Re-export new quickchr/service-endpoint/selection symbols from resolver barrel.
src/resolver/facts.ts Update resolver doc reference from expandCdbSelection to expandSelection.
src/index.ts Update public exports to use expandSelection / Selection* instead of expandCdbSelection / CdbSelection*.
src/execute.ts Rename comment reference from expandCdbSelection to expandSelection.
src/execute-fanout.ts Replace CDB-shaped selection types with neutral selection types in execute fan-out plumbing.
src/errors.ts Extend CentrsErrorCode union with quickchr/*.
src/core/error-catalog.ts Add catalog entries for quickchr/package-unavailable, quickchr/unsupported, quickchr/machine-not-found, quickchr/machine-stopped.
src/api-fanout.ts Replace CDB-shaped selection types with neutral selection types in api fan-out plumbing.
package.json Move @tikoci/quickchr from devDependencies to optionalDependencies and bump to ^0.4.4.
docs/errors/README.md Document the new quickchr/* error family.
docs/errors/quickchr/unsupported.md Add quickchr/unsupported error page stub.
docs/errors/quickchr/package-unavailable.md Add quickchr/package-unavailable error page stub.
docs/errors/quickchr/machine-stopped.md Add quickchr/machine-stopped error page stub.
docs/errors/quickchr/machine-not-found.md Add quickchr/machine-not-found error page stub.
docs/CONSTITUTION.md Add the normative “Resolution providers” subsection (Axis A/B + provider taxonomy + per-service map).
bun.lock Lockfile update to reflect @tikoci/quickchr@0.4.4 as optional dependency.

Comment thread src/resolver/service-endpoint.ts Outdated
Comment thread src/resolver/quickchr-provider.ts
Comment thread src/index.ts
Comment thread src/index.ts

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 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.

Inline comments:
In `@src/resolver/quickchr-provider.ts`:
- Around line 113-116: Update mapServices to validate that services is present
before calling Object.entries. When it is missing, throw an actionable error
that identifies the malformed descriptor and provides guidance to supply the
required services mapping, allowing the existing error wrappers to handle it.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 392a7536-6bd2-49e3-88c6-09c045e7697f

📥 Commits

Reviewing files that changed from the base of the PR and between f0b2f78 and 3e45888.

⛔ Files ignored due to path filters (1)
  • bun.lock is excluded by !**/*.lock
📒 Files selected for processing (29)
  • docs/CONSTITUTION.md
  • docs/errors/README.md
  • docs/errors/quickchr/machine-not-found.md
  • docs/errors/quickchr/machine-stopped.md
  • docs/errors/quickchr/package-unavailable.md
  • docs/errors/quickchr/unsupported.md
  • package.json
  • src/api-fanout.ts
  • src/core/error-catalog.ts
  • src/errors.ts
  • src/execute-fanout.ts
  • src/execute.ts
  • src/index.ts
  • src/resolver/facts.ts
  • src/resolver/index.ts
  • src/resolver/quickchr-provider.ts
  • src/resolver/selection.ts
  • src/resolver/service-endpoint.ts
  • src/retrieve-fanout.ts
  • src/transfer-fanout.ts
  • src/transfer.ts
  • test/unit/api-fanout.test.ts
  • test/unit/error-catalog.test.ts
  • test/unit/execute-fanout.test.ts
  • test/unit/quickchr-provider.test.ts
  • test/unit/retrieve-fanout.test.ts
  • test/unit/selection.test.ts
  • test/unit/service-endpoint.test.ts
  • test/unit/transfer-fanout.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (3)
  • GitHub Check: CHR smoke (stable)
  • GitHub Check: copilot-pull-request-reviewer
  • GitHub Check: Analyze (javascript-typescript)
🧰 Additional context used
📓 Path-based instructions (2)
src/**/*.{ts,tsx}

📄 CodeRabbit inference engine (src/AGENTS.md)

src/**/*.{ts,tsx}: Use Bun-native TypeScript and Web APIs where possible
Errors must be actionable for humans and agents, with next-step guidance when a dependency, protocol, credential, or validation source is missing

Files:

  • src/resolver/facts.ts
  • src/errors.ts
  • src/transfer.ts
  • src/core/error-catalog.ts
  • src/resolver/index.ts
  • src/index.ts
  • src/api-fanout.ts
  • src/resolver/quickchr-provider.ts
  • src/resolver/selection.ts
  • src/retrieve-fanout.ts
  • src/execute.ts
  • src/resolver/service-endpoint.ts
  • src/execute-fanout.ts
  • src/transfer-fanout.ts
docs/CONSTITUTION.md

📄 CodeRabbit inference engine (AGENTS.md)

docs/CONSTITUTION.md is the load-bearing source of truth for validation, envelope, errors, settings, identity, protocol selection, and done definition.

Files:

  • docs/CONSTITUTION.md
🪛 LanguageTool
docs/errors/quickchr/package-unavailable.md

[grammar] ~7-~7: Ensure spelling is correct
Context: ...ION.md`](../../CONSTITUTION.md) for the centrs error contract. This stub will be expan...

(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)

docs/errors/quickchr/unsupported.md

[grammar] ~3-~3: Ensure spelling is correct
Context: ... speaks a newer descriptor version than centrs understands. ## Fix See [`docs/CONSTI...

(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)


[grammar] ~7-~7: Ensure spelling is correct
Context: ...ION.md`](../../CONSTITUTION.md) for the centrs error contract. This stub will be expan...

(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)

docs/errors/quickchr/machine-stopped.md

[grammar] ~7-~7: Ensure spelling is correct
Context: ...ION.md`](../../CONSTITUTION.md) for the centrs error contract. This stub will be expan...

(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)

docs/errors/quickchr/machine-not-found.md

[grammar] ~7-~7: Ensure spelling is correct
Context: ...ION.md`](../../CONSTITUTION.md) for the centrs error contract. This stub will be expan...

(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)

docs/CONSTITUTION.md

[grammar] ~265-~265: Ensure spelling is correct
Context: ...ution has two independent axes, and centrs keeps them separate so the resolver is genera...

(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)

🔇 Additional comments (29)
src/resolver/selection.ts (1)

57-57: LGTM!

Also applies to: 75-98, 125-130, 236-236, 267-267

src/resolver/facts.ts (1)

3-3: LGTM!

src/index.ts (1)

341-341: LGTM!

Also applies to: 369-371

test/unit/selection.test.ts (1)

14-16: LGTM!

Also applies to: 77-89, 130-130, 211-211, 249-249, 267-272, 293-307, 308-314, 322-336

src/retrieve-fanout.ts (1)

15-15: LGTM!

Also applies to: 52-57, 110-115, 138-138, 162-162, 197-197, 237-237, 376-376

test/unit/retrieve-fanout.test.ts (1)

5-8: LGTM!

Also applies to: 32-32

src/api-fanout.ts (1)

10-10: LGTM!

Also applies to: 56-61, 95-98, 123-123, 142-142, 166-166, 212-212, 246-246, 305-305, 332-332

src/execute-fanout.ts (1)

12-12: LGTM!

Also applies to: 55-60, 94-97, 127-127, 142-142, 166-166, 198-198, 233-233, 295-295, 326-326

src/execute.ts (1)

290-290: LGTM!

src/transfer-fanout.ts (1)

11-11: LGTM!

Also applies to: 45-50, 104-110, 133-137, 168-172, 200-200, 220-220, 271-271, 311-323, 380-380, 410-410

src/transfer.ts (1)

822-822: LGTM!

test/unit/api-fanout.test.ts (1)

18-23: LGTM!

Also applies to: 66-68

test/unit/execute-fanout.test.ts (1)

16-21: LGTM!

Also applies to: 21-33, 69-71

test/unit/transfer-fanout.test.ts (1)

7-11: LGTM!

Also applies to: 25-25, 80-80

docs/CONSTITUTION.md (1)

263-296: LGTM!

src/resolver/quickchr-provider.ts (1)

1-112: LGTM!

Also applies to: 117-223

src/resolver/service-endpoint.ts (1)

1-140: LGTM!

src/resolver/index.ts (1)

91-114: LGTM!

package.json (1)

14-26: LGTM!

docs/errors/README.md (1)

50-52: LGTM!

docs/errors/quickchr/machine-not-found.md (1)

1-10: LGTM!

test/unit/service-endpoint.test.ts (1)

1-98: LGTM!

src/errors.ts (1)

16-16: LGTM!

src/core/error-catalog.ts (1)

237-256: LGTM!

docs/errors/quickchr/machine-stopped.md (1)

1-10: LGTM!

docs/errors/quickchr/package-unavailable.md (1)

1-10: LGTM!

docs/errors/quickchr/unsupported.md (1)

1-10: LGTM!

test/unit/error-catalog.test.ts (1)

25-25: LGTM!

test/unit/quickchr-provider.test.ts (1)

1-164: LGTM!

Comment thread src/resolver/quickchr-provider.ts

@mobileskyfi mobileskyfi left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Findings from a separate pass, focused on the provider seam and docs alignment. I would treat the first item as blocking before merge.

  1. src/resolver/quickchr-provider.ts:201 only rejects descriptor versions greater than SUPPORTED_DESCRIPTOR_VERSION, so an old-shaped descriptor with no descriptorVersion is accepted (undefined > 1 is false) and then src/resolver/quickchr-provider.ts:216 calls mapServices(descriptor.services). If that old descriptor also lacks services (the exact pre-v1 compatibility case this PR says it handles), centrs throws a raw TypeError: Object.entries requires that input parameter not be null or undefined instead of the promised typed quickchr/unsupported error. I reproduced this locally with an injected loader returning { name, version, arch }. The fix should validate the descriptor shape before mapping: descriptorVersion must be a supported number/version, services must be an object, and missing/old/malformed v1 fields should become an actionable CentrsError (quickchr/unsupported) with a unit test. I would not silently convert missing services to an empty map; the quickchr contract makes services required, so empty would hide a provider contract violation and defer the failure to a later --via lookup.

  2. src/resolver/service-endpoint.ts:128 makes every key in ServiceEndpointMap an AnyServiceEndpoint, so services.ssh loses the SSH-specific guarantee that an available SSH endpoint has auth: SshEndpointAuth with batchModes. That weakens the exact Phase 4 contract that will gate --via ssh / transfer --via sftp. I would shape the map by service key ("rest-api"?: ServiceEndpoint, "native-api"?: ServiceEndpoint, ssh?: SshServiceEndpoint, with unsupported/future protocol keys absent or intentionally broad if needed) and make isEndpointAvailable preserve the input union via Extract<T, { available: true }>.

  3. The new docs/errors/quickchr/*.md pages are still generated stubs (This stub will be expanded...). The path-scoped actionable-error instruction says new error pages should be enriched with the typical trigger and remediation after bun run docs:errors. Since docs/errors/README.md now advertises the quickchr/* family, these pages should carry the actual user/agent guidance in this PR: missing optional package install/workaround, unsupported old/malformed/newer descriptor, machine name not found, and stopped machine/start command.

Non-findings: the no-alias decision for the expandCdbSelection rename is fine given the repo's current consumer posture; I do not see a docs/MATRIX.md or per-command README change required yet because this PR deliberately stops before wiring a user-visible --quickchr flag.

Verification I ran: bun run lint, bun test, bun run build, plus the old-shaped descriptor repro described above. GitHub checks are also green at PR head 3e458884b3c4e7fb6ef1d50d40d46fba80157ad2.

…, load-error split (#134 review)

Address PR #181 review findings (Copilot, CodeRabbit, Codex):

- Descriptor shape validation before mapping (Codex #1 / CodeRabbit): an
  old/pre-v1 descriptor with no `descriptorVersion` and no `services` now
  raises a typed `quickchr/unsupported` instead of a raw
  `TypeError: Object.entries(undefined)`. Missing `services` is rejected, not
  silently mapped to `{}`, so a provider contract violation surfaces at
  resolve time rather than a later `--via` lookup. Descriptor now crosses the
  optional-dependency boundary as `unknown` until `validateDescriptor()`.

- Per-key `ServiceEndpointMap` + generic `isEndpointAvailable` (Codex #2 /
  Copilot #1): `map.ssh` keeps its `SshServiceEndpoint` type, so narrowing
  preserves the `auth: SshEndpointAuth`/`batchModes` guarantee Phase 4 gates
  `--via ssh`/`transfer --via sftp` on — no cast/`"batchModes" in` probe.

- Load-error split (Copilot #2): only a genuine module-not-found maps to
  `quickchr/package-unavailable`; an installed package that throws on load is
  a real fault → `quickchr/unsupported` preserving the cause.

- Enrich the four `docs/errors/quickchr/*.md` pages with typical trigger and
  remediation (Codex #3 / actionable-errors instruction).

No-alias decisions for the `expandCdbSelection`/`CdbSelection*` rename
(Copilot #3/#4) declined by the maintainer — nothing depends on centrs as a
library. New unit tests cover every added error path.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@mobileskyfi

Copy link
Copy Markdown
Contributor Author

Thanks all — three reviews (Copilot, CodeRabbit, and a Codex pass) triaged. Fixes pushed in 2298e5f. Disposition:

Fixed

  • Descriptor shape validation (Codex build(deps-dev): bump has-flag from 4.0.0 to 5.0.1 in the npm group #1 blocking / CodeRabbit inline on mapServices). An old/pre-v1 descriptor with no descriptorVersion (so undefined > 1 is false) and no services used to reach Object.entries(undefined) and throw a raw TypeError. Now the descriptor crosses the optional-dependency boundary as unknown and passes through validateDescriptor() first; every malformed/pre-v1/newer case becomes a typed quickchr/unsupported. I went with Codex's stance over CodeRabbit's return map autofix: a missing services is rejected, not silently mapped to {} — an empty map would hide a provider contract violation and defer the failure to a later --via lookup. New unit tests cover pre-v1 (no descriptorVersion), missing services, and non-object descriptor.

  • Per-key ServiceEndpointMap + generic guard (Codex chore(deps-dev): bump @biomejs/biome from 2.4.13 to 2.4.14 in the npm group across 1 directory #2 / Copilot on service-endpoint.ts:139). ServiceEndpointMap is now keyed per-service, so map.ssh stays a SshServiceEndpoint; isEndpointAvailable<T> narrows via Extract<T, { available: true }>. An available SSH endpoint keeps its required auth: SshEndpointAuth/batchModes — the exact Phase 4 gate for --via ssh / transfer --via sftp — with no cast or "batchModes" in probe. The service-endpoint test now anchors that narrowing directly.

  • Load-error split (Copilot on quickchr-provider.ts:146). Only a genuine module-not-found maps to quickchr/package-unavailable; an installed package that throws during evaluation is a real fault → quickchr/unsupported preserving the cause. Test added for the installed-but-throws path.

  • Enriched error pages (Codex Potential fix for 1 code quality finding #3 / actionable-errors instruction). The four docs/errors/quickchr/*.md now carry typical trigger + remediation instead of the generator stub.

Declined (maintainer call)

Notes

  • CodeRabbit's Description check / Docstring coverage pre-merge warnings are template/threshold advisories on an intentionally-incremental PR; not blocking and left as-is.
  • Verification: bun run lint (biome + tsc), bun test (1175 pass / 0 fail, +4 new), bun run build, bun run lint:ci all green.

Scope is unchanged — this still lands Phases 1–2 only and does not close #134; Phase 3+ tracked there.

@mobileskyfi mobileskyfi left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-review after 2298e5f: the original three Codex findings look addressed. Descriptor shape validation now fails closed with quickchr/unsupported, the ssh service keeps its SSH-specific endpoint type after narrowing, and the quickchr error pages are no longer stubs.

One new follow-up from the load-error split:

  1. src/resolver/quickchr-provider.ts:128-134 still treats any ERR_MODULE_NOT_FOUND / MODULE_NOT_FOUND (or any "cannot find module/package" message) as quickchr/package-unavailable. That can misclassify an installed @tikoci/quickchr package whose own module evaluation fails because a transitive import is missing. I reproduced this through the existing load seam by throwing Object.assign(new Error("Cannot find package 'left-pad' imported from /node_modules/@tikoci/quickchr/src/index.ts"), { code: "ERR_MODULE_NOT_FOUND" }); resolveQuickchrTarget() reports quickchr/package-unavailable with the summary saying @tikoci/quickchr is not installed. That contradicts the new docs' "only when the module is genuinely not found" distinction and sends the user to reinstall/add the wrong package. The classifier should check that the missing specifier is actually @tikoci/quickchr (or otherwise only map the top-level import target to package-unavailable); transitive/module-evaluation not-found errors should remain quickchr/unsupported with the original cause.

Verification I ran on head 2298e5feaa9e37fb6e3ec57b6c42cfcd76462873: bun run lint, targeted quickchr/service-endpoint tests, bun run build, bun test (1175 pass / 34 skip), and bun run lint:ci. GitHub checks are also green.

…ode alone (#134 review)

Follow-up Codex finding on the load-error split: `isModuleNotFound` mapped any
ERR_MODULE_NOT_FOUND / "cannot find module|package" to
`quickchr/package-unavailable`. An installed `@tikoci/quickchr` whose own
transitive import is missing carries the same code but names a different
specifier — mislabeling it "not installed" sends the user to reinstall the
wrong package.

Now parse the quoted specifier the loader could not resolve and only treat a
failure to find `@tikoci/quickchr` *itself* as package-unavailable; a
transitive/module-evaluation not-found stays `quickchr/unsupported` with the
cause preserved. Tests cover both the transitive repro and the genuine
package-missing case.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@mobileskyfi

Copy link
Copy Markdown
Contributor Author

Re-review follow-up (Codex, 22:24) addressed in 9dce4a8:

Module-not-found misclassificationisModuleNotFound no longer keys off the ERR_MODULE_NOT_FOUND code alone. It now parses the specifier the loader could not resolve and only treats a failure to find @tikoci/quickchr itself as quickchr/package-unavailable. A transitive dep missing inside an installed quickchr (your left-pad/ERR_MODULE_NOT_FOUND repro) — or any module-evaluation fault — stays quickchr/unsupported with the cause preserved, matching the docs' "only when the module is genuinely not found" distinction. Tests cover both the transitive repro and the genuine package-missing case.

All five inline review threads (Copilot ×4, CodeRabbit ×1) are resolved. Verification on 9dce4a8: bun run lint, bun test (1177 pass / 0 fail, +2), bun run build all green. CI re-running on the new head.

@mobileskyfi
mobileskyfi merged commit 1f6da89 into main Jul 18, 2026
10 checks passed
@mobileskyfi
mobileskyfi deleted the feat/resolution-provider-tier1-134 branch July 18, 2026 22:33
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.

--quickchr <name>: resolve quickchr VMs as (fan-out-able) targets via dynamic import

2 participants