Conversation
start integration tests with effect lint (CLI-2414)There was a problem hiding this comment.
🤖 AI Review
All eight findings were verified against the PR and trusted conventions. Five quality findings are confirmed: valuable fixture rationale was removed, two route callbacks redundantly provide filesystem services, 72 unnecessary suspend wrappers were added, three generators are needlessly nested, and one assertion no longer excludes timeout tags from non-Fail causes. Three findings are refuted because the existing service provision, strict generated-RSA fixture, and test-finalizer defect handling are valid. No major functional or security regression was found.
Findings
| Severity | Location | Category | Sources | Claim |
|---|---|---|---|---|
| 🟡 MINOR | apps/cli/src/commands/start/start.integration.test.ts:2238 |
documentation |
claude | The refactor removes numerous comments explaining non-obvious fixture choices, leaving important test rationale undocumented. |
| 🟡 MINOR | apps/cli/src/commands/start/start.integration.test.ts:3527 |
test-coverage |
claude | The replacement assertion only excludes HealthCheckTimeoutError from Fail reasons, weakening the prior whole-cause absence check. |
| ⚪ NIT | apps/cli/src/commands/start/start.integration.test.ts:1056 |
test-quality |
claude | Two route callbacks shadow the enclosing filesystem service and redundantly provide BunServices for every invocation. |
| ⚪ NIT | apps/cli/src/commands/start/start.integration.test.ts:2244 |
consistency |
claude | The 72 Effect.suspend wrappers around withEnvVar bodies are redundant and inconsistent with the equivalent direct Effect.gen usage. |
| ⚪ NIT | apps/cli/src/commands/start/start.integration.test.ts:2342 |
readability |
claude | Three tests unnecessarily nest a second Effect.gen behind return yield*. |
Refuted findings (kept for transparency, not posted as review comments)
apps/cli/src/commands/start/start.integration.test.ts:4028(consistency): The Twilio override test incorrectly lacks an outer Effect.provide(BunServices.layer).
Refuted: All filesystem-dependent work is already inside the provided inner generator, and the command layer also includes BunServices. Adding an outer provider would only duplicate an existing provision.apps/cli/src/commands/start/start.integration.test.ts:1570(test-quality): The explicit RSA private-JWK schema is brittle and should be replaced with Schema.Unknown.
Refuted: The fixture is not arbitrary input: it is a generated private RSA JWK, whose required material is exactly kty, n, e, d, p, q, dp, dq, and qi. Validating that shape makes the signing fixture explicit; other uses of Schema.Unknown do not establish that strict validation is erroneous here.apps/cli/src/commands/start/start.integration.test.ts:2828(error-handling): Using Effect.orDie in the two filesystem cleanup finalizers improperly converts recoverable PlatformError failures into defects.
Refuted: These are test teardown finalizers, not operational recovery paths. Finalizers require an infallible typed error channel, and turning an unexpected cleanup failure into a visible test defect matches existing trusted repository practice rather than swallowing the failure.
Stats
Claude findings: 7 · Codex findings: 1 · Confirmed: 5 · 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.
…start-integration-effect-lint
TL;DR
brings the
supabase starthandler integration suite under the effect lint,completing coverage of the command family
whats introduced?
FileSystem/Pathservices, and env cases use the sharedwithEnvVarhelperCause.prettyref: