Skip to content

refactor(cli): cover bootstrap with effect lint (CLI-2439) - #6636

Merged
7ttp merged 4 commits into
developfrom
7ttp/cli-2439-bootstrap-family-coverage
Sep 16, 2026
Merged

7ttp merged 4 commits into
developfrom
7ttp/cli-2439-bootstrap-family-coverage

Conversation

@7ttp

@7ttp 7ttp commented Sep 15, 2026

Copy link
Copy Markdown
Member

TL;DR

brings the supabase bootstrap command family under the effect lint

whats introduced?

effect lint applied to bootstrap:

  • one allow list entry in .oxlintrc.effect.json
  • the ambient SUPABASE_DB_PASSWORD and GITHUB_TOKEN reads move onto CommandSettings as redacted optional fields, and the raw SUPABASE_WORKDIR value rides along verbatim, all captured the same way as the access token
  • the .env.example parse failure becomes a typed error with its telemetry tag registered
  • the samples listing decodes through a schema codec
  • integration tests pin the settings sourced create password and the env configured workdir

ref:

@7ttp 7ttp self-assigned this Sep 15, 2026
@7ttp
7ttp requested a review from a team as a code owner September 15, 2026 11:06

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🤖 AI Review

Codex reported no findings. Of Claude's eight findings, six are confirmed and two refuted. Confirmed issues comprise one untyped template-decoding defect, two test-coverage gaps, one test-isolation concern, one misleading comment, and one non-total Effect thunk. No critical or major defects were verified.

Findings

Severity Location Category Sources Claim
🟡 MINOR apps/cli/src/commands/bootstrap/bootstrap.templates.ts:138 error-handling claude The samples codec accepts any JSON value, after which property access on null can throw an untyped defect instead of BootstrapTemplateListError.
🟡 MINOR apps/cli/src/commands/bootstrap/bootstrap.templates.ts:78 test-coverage claude The settings-sourced authorization header and schema-based samples decoding have no direct service-level tests.
🟡 MINOR apps/cli/src/commands/bootstrap/bootstrap.workdir-cache.integration.test.ts:232 test-isolation claude The test constructs the real command settings layer without fully isolating process environment variables, allowing ambient profile or credential values to leak into it.
🟡 MINOR apps/cli/src/commands/bootstrap/bootstrap.handler.ts:155 effect-idiom claude process.chdir is a throwing operation wrapped in Effect.sync, so a filesystem race or permission failure becomes a defect rather than a typed bootstrap failure.
⚪ NIT apps/cli/src/commands/bootstrap/bootstrap.integration.test.ts:346 documentation claude The comment incorrectly attributes prompting to explicitWorkdir, although the handler does not consult that field.
⚪ NIT apps/cli/src/commands/bootstrap/bootstrap.integration.test.ts:355 test-coverage claude The test named as using SUPABASE_WORKDIR without prompting does not assert that no prompt occurred.

Findings outside the diff

  • 🟡 MINOR apps/cli/src/commands/bootstrap/bootstrap.handler.ts:155process.chdir is a throwing operation wrapped in Effect.sync, so a filesystem race or permission failure becomes a defect rather than a typed bootstrap failure.
Refuted findings (kept for transparency, not posted as review comments)
  • apps/cli/src/config/command-settings.layer.ts:147 (consistency): Capturing DB and GitHub credentials in CommandSettings conflicts with remaining lazy environment reads and creates contradictory sources of truth.
    Refuted: The code defines distinct lifecycles rather than contradictory sources: bootstrap consumes captured settings, while db-config intentionally performs per-invocation resolution. During bootstrap, resolveLinkedConn receives Option.some(created.dbPassword), so its ambient fallback cannot override the captured/created password.
  • apps/cli/src/commands/bootstrap/bootstrap.suggest.ts:1 (style): Using import { type Path } creates an inappropriate value import and violates an established repository convention requiring import type.
    Refuted: The inline type modifier makes Path type-only and produces no runtime value import. Other files' use of import type does not establish a documented rule against this semantically equivalent syntax.

Stats

Claude findings: 8 · Codex findings: 0 · Confirmed: 6 · 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.

Comment thread apps/cli/src/commands/bootstrap/bootstrap.templates.ts
Comment thread apps/cli/src/commands/bootstrap/bootstrap.templates.ts
Comment thread apps/cli/src/commands/bootstrap/bootstrap.integration.test.ts Outdated
Comment thread apps/cli/src/commands/bootstrap/bootstrap.integration.test.ts
…otstrap-family-coverage

# Conflicts:
#	apps/cli/src/auth/command-platform-api.layer.unit.test.ts
#	apps/cli/src/command-internal/edge-runtime-script.layer.integration.test.ts
#	apps/cli/src/commands/services/services.integration.test.ts
#	apps/cli/src/config/command-settings.layer.ts
#	apps/cli/src/config/command-settings.layer.unit.test.ts
#	apps/cli/src/config/project-ref.layer.unit.test.ts
#	apps/cli/tests/helpers/command-mocks.ts
#	apps/cli/tests/helpers/compute.ts
@7ttp
7ttp enabled auto-merge September 16, 2026 09:26
@7ttp
7ttp added this pull request to the merge queue Sep 16, 2026
Merged via the queue into develop with commit c5d2983 Sep 16, 2026
21 checks passed
@7ttp
7ttp deleted the 7ttp/cli-2439-bootstrap-family-coverage branch September 16, 2026 09:40
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.

2 participants