refactor(cli): cover functions download with effect lint (CLI-2459) - #6713
Merged
Merged
Conversation
Contributor
There was a problem hiding this comment.
🤖 AI Review
Both independent reviews were available. Of Claude's three nit findings, one duplication concern is confirmed and two style/readability claims are refuted by existing repository patterns. Codex reported no findings. No correctness, security, or user-impacting issues were identified.
Findings
| Severity | Location | Category | Sources | Claim |
|---|---|---|---|---|
| ⚪ NIT | apps/cli/src/commands/functions/download/download.live.test.ts:16 |
duplication |
claude | The Effect port duplicates the function-cleanup helper and tagged-error pattern across a third functions live test instead of sharing the common responsibility. |
Refuted findings (kept for transparency, not posted as review comments)
apps/cli/src/commands/functions/download/download.integration.test.ts:1916(style): The threewithEnvVarcall sites use an inline pipe lambda instead of the repository's prevailing direct-wrapper form, making the environment scope harder to see.
Refuted: The claimed convention is not borne out by the checked-out code: multiple existing tests use the same inline lambda form, including an exactEffect.provide(layer), (body) => withEnvVar(...)pattern.apps/cli/src/commands/functions/download/download.integration.test.ts:953(readability): ReplacingJSON.stringify({ project_id: "json-project" })with a handwritten JSON literal loses self-documenting structure.
Refuted: The one-property literal exposes the exact key and value directly, its purpose is explicit from the surrounding filename and assertions, and literal JSON fixtures are an established nearby pattern consistent with the trusted guidance to keep test fixtures concise.
Stats
Claude findings: 3 · Codex findings: 0 · Confirmed: 1 · Refuted: 2 · 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.
Coly010
approved these changes
Sep 23, 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
supabase functions download, the last uncovered subcommand, under the effect lintwhats introduced?
effect lint applied to
download, so the wholefunctionsfamily is covered now:functionsallow list entries in.oxlintrc.effect.jsoncollapse into one
!apps/cli/src/commands/functions/**, which adds exactly the fivedownloadfilesPathservice instead ofnode:pathFileSystem/PathserviceswithEnvVarinstead of setting and restoringprocess.envby hand, and the ambient env assertion reads throughConfigover aConfigProvider.fromEnvJSON.stringifyrunSupabaseEffectharness withwithTempHome, live tests through thecliEffectfixtureref: