refactor(cli): cover start runtime with effect lint (CLI-2414) - #6676
Merged
Merged
Conversation
Member
Author
|
/ai-review |
Contributor
There was a problem hiding this comment.
🤖 AI Review
Both reviews completed. After deduplication, 11 findings are confirmed and 3 are refuted. No critical or major issues were verified; the confirmed issues are minor error-handling, test-coverage, lint-scope, and test-layer concerns plus documentation/style nits. Codex’s two findings duplicate confirmed Claude findings and are merged below.
Findings
| Severity | Location | Category | Sources | Claim |
|---|---|---|---|---|
| 🟡 MINOR | apps/cli/src/commands/start/start.handler.ts:356 |
error-handling |
claude | A PlatformError without reason.cause produces a degraded email-template read error such as [object Object]. |
| 🟡 MINOR | apps/cli/src/commands/start/start.integration.test.ts:3770 |
test-coverage |
claude | The first five new environment-precedence scenarios do not exercise resolveAmbientEnvValues or production preserveEmptyStrings behavior. |
| 🟡 MINOR | apps/cli/src/commands/start/services/edge-runtime.service.integration.test.ts:154 |
testing |
claude | All 17 test bodies redundantly provide BunServices.layer even though the enclosing Effect test suite already supplies it. |
| 🟡 MINOR | apps/cli/src/commands/start/start.handler.ts:186 |
error-handling |
claude | resolveAmbientEnvValues leaves ConfigError in start’s error channel instead of mapping it to a Start domain error. |
| 🟡 MINOR | .oxlintrc.effect.json:52 |
maintainability |
claude | The PR enables Effect lint for the start command but re-excludes its main integration test and all three start E2E test files. |
| ⚪ NIT | apps/cli/src/commands/start/services/kong.service.ts:29 |
documentation |
claude+codex | The Kong network-alias JSDoc now attaches to KongPaths rather than KONG_NETWORK_ALIASES. |
| ⚪ NIT | apps/cli/src/commands/start/services/kong.service.ts:30 |
api-design |
claude | KongPaths is private despite appearing in two exported function signatures, and its wrapper shape differs from sibling path-injected builders. |
| ⚪ NIT | apps/cli/src/commands/start/services/edge-runtime.service.integration.test.ts:117 |
comments |
claude+codex | The same explanatory comment is duplicated consecutively. |
| ⚪ NIT | apps/cli/src/commands/start/services/edge-runtime.service.integration.test.ts:1 |
style |
claude | The file has two separate imports from effect. |
| ⚪ NIT | apps/cli/src/commands/start/lib/env-or-default.ts:8 |
style |
claude | Optional chaining on ambientEnvValues is redundant because the parameter defaults to a non-null object. |
| ⚪ NIT | apps/cli/src/commands/start/start.handler.ts:1814 |
consistency |
claude | One return yield* Effect.fail(...) remains while neighboring failures use direct yielding of yieldable tagged errors. |
Findings outside the diff
- ⚪ NIT
apps/cli/src/commands/start/start.handler.ts:1814— One return yield* Effect.fail(...) remains while neighboring failures use direct yielding of yieldable tagged errors.
Refuted findings (kept for transparency, not posted as review comments)
apps/cli/src/commands/start/services/edge-runtime.service.integration.test.ts:122(testing): The beforeEach hook unnecessarily starts a nested Effect runtime despite the enclosing suite layer.
Refuted: The enclosing test layer is not ambient in a plain Vitest beforeEach hook. Effect.runPromise with an explicitly provided layer is the hook’s Promise boundary, not redundant access to the suite runtime.apps/cli/src/commands/start/start.handler.ts:657(typing): Using void for the missing-container branch inaccurately models a runtime undefined value.
Refuted: void is the exact success type produced by Effect.void and correctly represents the ignored-value branch; there is no typing or runtime mismatch.apps/cli/src/commands/start/start.handler.ts:1555(consistency): Two remaining process.env reads make the new Config-based ambient-environment conversion inconsistent.
Refuted: The remaining reads are deliberate, distinct boundaries: one must enumerate dynamically named variables, and the other is already dependency-injectable. They do not undermine the fixed-key Config migration.
Stats
Claude findings: 14 · Codex findings: 2 · Confirmed: 11 · Refuted: 3 · Uncertain: 0
Models: claude-opus-5 + gpt-5.6-sol · Trigger: manual · Workflow run
This review runs once per PR. A maintainer can request another with a /ai-review comment.
Base automatically changed from
7ttp/cli-2460-cover-the-shared-functions-runtime-with-effect-lint
to
develop
September 21, 2026 07:31
Coly010
approved these changes
Sep 21, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
TL;DR
brings the
supabase startruntime and service tests under the effect lintwhats introduced?
FileSystem/Pathservices, preserving eager validation and existing errorsprocess.envdirectly, preserving precedence, empty values, and Windows case insensitive lookupref: