refactor(cli): resolve hostnames through Effect services - #6634
Conversation
There was a problem hiding this comment.
🤖 AI Review
The reviews yield one critical confirmed defect: explicit db diff --from/--to mode crashes when either ref is local because runtimeInfo is accessed before initialization. Eight additional minor/nit concerns are confirmed, chiefly test isolation/coverage, unmapped hostname configuration errors, stale documentation, and cleanup issues. The claimed DbConfigResolver type error is refuted because DbConfigError explicitly includes Config.ConfigError; both formatting claims are also refuted by the checked-out code's correct alignment.
Findings
| Severity | Location | Category | Sources | Claim |
|---|---|---|---|---|
| 🔴 CRITICAL | apps/cli/src/commands/db/diff/diff.handler.ts:259 |
correctness |
claude | Explicit diff mode crashes with a temporal-dead-zone ReferenceError when resolving a local ref because resolveRef reads runtimeInfo before its declaration is initialized. |
| 🟡 MINOR | apps/cli/src/command-internal/hostname.unit.test.ts:77 |
test-isolation |
claude | The empty-override assertion is environment-dependent because it supplies no hermetic ConfigProvider and therefore can resolve an ambient Docker host or context instead of loopback. |
| 🟡 MINOR | apps/cli/src/command-internal/hostname.unit.test.ts:275 |
test-coverage |
claude+codex | The proxy-bypass suite no longer covers unset proxy variables, preferred lowercase no_proxy, or empty-lowercase fallback behavior. |
| 🟡 MINOR | apps/cli/src/commands/db/schema/declarative/sync/sync.handler.ts:699 |
error-handling |
claude | Hostname Config.ConfigError failures escape without being translated into command-domain errors in declarative sync and local type generation. |
| ⚪ NIT | apps/cli/src/command-internal/hostname.ts:145 |
api-design |
claude | The projectEnvValues override parameter is unused by production callers and misleadingly suggests that project dotenv values may supply Docker-client configuration. |
| ⚪ NIT | apps/cli/src/command-internal/hostname.ts:76 |
documentation |
claude | The rewrite removes concise explanations for two non-obvious Docker behaviors: the default context has no context-store entry, and an unreadable non-default context intentionally yields no endpoint so callers can fall back. |
| ⚪ NIT | apps/cli/src/shared/functions/functions-config.ts:2 |
code-style |
claude | Several Effect service namespaces are imported as runtime values despite being used only in type positions. |
| ⚪ NIT | apps/cli/src/command-internal/db-config.integration.test.ts:103 |
test-quality |
claude | The resolver test helper uses an unnecessary IIFE and provisions ConfigProvider at two different levels, obscuring that execution-time hostname reads require the outer provider. |
| ⚪ NIT | apps/cli/src/command-internal/db-bootstrap/local-db-running.ts:234 |
documentation |
claude | The layer documentation still says endpoint resolution occurs inside Effect.suspend, although the implementation no longer uses that combinator. |
Findings outside the diff
- ⚪ NIT
apps/cli/src/command-internal/db-bootstrap/local-db-running.ts:234— The layer documentation still says endpoint resolution occurs insideEffect.suspend, although the implementation no longer uses that combinator.
Refuted findings (kept for transparency, not posted as review comments)
apps/cli/src/commands/experimental/stack/start/start.integration.test.ts:207(formatting): The addedEffect.providepipe argument is misindented and would fail formatting checks.
Refuted: The checked-out file contains correctly aligned sibling pipe arguments; the cited indentation defect is not present.apps/cli/src/commands/experimental/stack/status/status.integration.test.ts:198(formatting): The added Bun and runtime layers are misindented in twoLayer.mergeAllcalls.
Refuted: Direct inspection of both cited call sites shows consistent argument indentation.apps/cli/src/command-internal/db-config.layer.ts:464(type-safety): The resolver leaksConfig.ConfigErroroutside the declaredDbConfigErrorunion and therefore does not type-check.
Refuted: The claimed type incompatibility is directly contradicted byConfig.ConfigErrorbeing a member of the resolver's declared error union.
Stats
Claude findings: 10 · Codex findings: 4 · Confirmed: 9 · Refuted: 3 · Uncertain: 0
Models: claude-opus-5 + gpt-5.6-sol · Trigger: auto · Workflow run
This review runs once per PR. A maintainer can request another with a /ai-review comment.
897dddd to
b5ceb67
Compare
6c8d495 to
8997db7
Compare
8997db7 to
910eb9c
Compare
910eb9c to
56edfc8
Compare
Tracing currently writes files synchronously when a span ends, while telemetry setup reads environment variables and generates identifiers outside Effect services. Use a scoped queue worker to export spans sequentially and drain pending exports during shutdown, including command failure and interruption, with a two-second deadline for stalled exporters. Filesystem writes, retention timestamps, encoding, environment signals, and session identifiers now use Effect services and schemas. Debug output and file export keep independent best-effort failure handling, and analytics destinations continue to come from ambient configuration. Based on supabase#6634.
Docker hostname discovery reads process environment and Docker context files synchronously, which prevents commands from supplying an Effect configuration provider and hides their filesystem and runtime requirements.
Resolve Docker environment settings with
Config, represent an absent context endpoint withOption, and read context metadata through Effect filesystem, path, and crypto services. Propagate those requirements to command composition while preserving Docker context precedence, platform socket defaults, and IPv4/IPv6 hostname behavior.Pass loaded project environment values through hostname resolution so type generation and storage URLs preserve local dotenv overrides while shell values retain precedence.
Based on #6631.