Repository navigation
chore(repo): sync develop into next - #7201
Conversation
…7150) ## TL;DR brings `upgrade-notice` to effect & the effect lint ## whats introduced? - the check reads files through `FileSystem`, the latest release through `HttpClient`, env through `Config` and the working and home directories through `RuntimeInfo` - `runCli`'s post-success hook now takes these services from the cli runtime instead of providing its own - release-lookup failures are typed as `UpgradeNoticeFetchError` - the cache write keeps the final entry link-safe without `O_NOFOLLOW`: the old entry is removed, which deletes a symlink or hard link itself, and the file is recreated exclusively with mode `0644` - so a stale read-only cache file is now replaced, and a stale cache file in a read-only cache directory no longer refreshes, so each command checks the release again ## ref - closes: CLI-2648
…s with effect lint (CLI-2655) (#7167) ## TL;DR brings the `db-bootstrap`, `stack`, `start` and `test-db` modules under the effect lint, tests included ## whats introduced? - `db-bootstrap/**`, `stack-*`, `start-*` and `test-db.*` join the allow list, with `db-bootstrap/local-db-running` kept out that for now since it probes the docker engine socket through `node:http` - `start-secrets-cleanup` removes staged secrets through `FileSystem`, and `test-db` builds its `pg_prove` args through `Path`, instead of `node:fs` and `node:path` - the stack shadow-cache key is encoded through `Schema` with the same bytes, and yieldable errors are yielded directly instead of through `Effect.fail` - the tests swap `node:` imports and `JSON` for effect services and schemas, and the start-secrets traversal test now aims at a real canary outside the staging root ## ref - closes: CLI-2655
# Conflicts: # .oxlintrc.effect.json # apps/cli/src/command-internal/db-bootstrap/db-setup.unit.test.ts # apps/cli/src/command-internal/db-bootstrap/shadow-cache.integration.test.ts # apps/cli/src/command-internal/db-bootstrap/start-local-database.ts # apps/cli/src/command-internal/stack-storage.native.integration.test.ts # apps/cli/src/command-internal/test-db.handler.ts # apps/cli/src/command-internal/upgrade-notice.integration.test.ts # apps/cli/src/command-internal/upgrade-notice.ts # apps/cli/src/command-internal/upgrade-notice.unit.test.ts
Claude resolved the merge of
|
| File | Resolution |
|---|---|
.oxlintrc.effect.json |
Dropped the workdir-search.ts entry because next deleted that file. Kept develop's two exclusions for db-bootstrap/local-db-running*, placed after the ! include entries so they still take effect. |
apps/cli/src/command-internal/db-bootstrap/start-local-database.ts |
Kept next's simpler checks on config.studio and config.local_smtp (the env-override checks are gone). Applied develop's lint fix to them: return yield* new DbConfigLoadError(...) instead of yield* Effect.fail(...). |
apps/cli/src/command-internal/test-db.handler.ts |
Took next's network name, supabase_network_${toml.projectId}. On next, readDbToml already returns projectId as a plain string, so develop's Option fallback plus sanitizing no longer applies. |
apps/cli/src/command-internal/db-bootstrap/shadow-cache.integration.test.ts |
Kept next's nesting (the cache env wraps the SUPABASE_HOME override) and its yield* shadowCacheDir(path). Used develop's nodePath.join and its DateTime-based anHourAgo. |
apps/cli/src/command-internal/db-bootstrap/db-setup.unit.test.ts |
Kept develop's Effect-native test bodies (FileSystem and makeWorkdir effects instead of rmSync). Applied next's shorter test titles without Go references, and reformatted the two tests whose titles now fit on one line. |
apps/cli/src/command-internal/stack-storage.native.integration.test.ts |
Kept develop's removal of the duplicate BunServices.layer from mergeAll, since provideMerge already supplies it. Added next's cliConfigValuesTestLayer. |
apps/cli/src/command-internal/upgrade-notice.ts |
Kept develop's Effect-native version (FileSystem/Path, HttpClient release lookup, Config-based env, RuntimeInfo, actionability error). Removed the delegatedToGo branch, made the ancestor walk accept supabase/config.json as well as config.toml, and read dotenv files with readFileString so a leading BOM is dropped. Imported candidateDotenvFilenames from cli-config-env.ts and removed next's now-unused UPGRADE_NOTICE_ENV_NAMES, readShellEnvironment and async fetch. |
apps/cli/src/command-internal/upgrade-notice.integration.test.ts |
Took develop's Effect-native version (withConfigEnv, runtimeInfoLayer, an Effect release stub). It already supplies the ConfigProvider, so next's cliConfigProviderLayer is not needed. |
apps/cli/src/command-internal/upgrade-notice.unit.test.ts |
Kept develop's Effect-native suites. Removed the delegatedToGo option, its doc comment, the delegated-to-Go test, and the delegatedToGo: false argument. Ported next's new test that a .env with a BOM still opts out. Dropped next's runHookWithProvider tests, which develop's release lookup suite already covers through the ambient ConfigProvider. |
Decisions for @supabase/cli. Confirm them, or push a change to this branch:
upgradeNoticeHookenv and GITHUB_TOKEN source:next'sreadShellEnvironmentwith the token passed to an async fetch, ordevelop'sConfig-based reads with an HttpClient fetch? Chose:develop'sConfig.option/Config.Redactedreads with the Effect HttpClient lookup. Both read the same ambient ConfigProvider, sonext's goal of not usingprocess.envstill holds, anddevelop's tests already cover the token and opt-out cases.- Dotenv BOM handling in the upgrade notice:
develop'sreadUtf8, which keeps the BOM, ornext's BOM-stripping decode? Chose:next's behaviour viafs.readFileString, which strips the BOM, for dotenv files only. The cache-tag read still usesreadUtf8, so a BOM-prefixed cached tag is still rejected asdevelopintends.
Quick checks. pnpm run check:all --continue (exit 1) failed on the merged tree.
Claude's fix. The effect lint check failed because develop added effect lint coverage for db-bootstrap, and a test merged in from next wrapped a single return yield* in an Effect.gen. I removed that wrapper in local-container-inputs.precedence.integration.test.ts and now call buildLocalDbContainerInputs(...).pipe(Effect.provide(layer)) directly; there were no review findings to answer.
Approve to fast-forward next once checks pass (workflow run).
![depthfirst-app[bot]](https://avatars.githubusercontent.com/in/1021002?s=60&v=4){kind=small}
| return Effect.void; | ||
| } | ||
| /* Interruptible even in the start and stop finalizers, so the first failure ends cleanup. */ | ||
| return Effect.interruptible(fs.remove(target, { recursive: true, force: true })); |
There was a problem hiding this comment.
⚪ Severity: LOW
While the change appears deliberate, SIGINT/SIGTERM rollback and stop invoke this cleanup from finalizers after container removal; making deletion interruptible lets pending cancellation stop the parallel removals. Staged JWT, service-role, and configured-secret files can therefore remain in the project tree for later same-account processes to read.
Helpful? Add 👍 / 👎
💡 Fix Suggestion
Suggestion: Remove the Effect.interruptible wrapper around fs.remove so the deletion inherits the uninterruptible context of the start rollback and stop finalizers. If you want to be sure it finishes even when called from an interruptible context, wrap it in Effect.uninterruptible instead. The cleanup already ends in Effect.ignore, so failures are swallowed either way. Update or remove the comment on line 30, which wrongly says interruptibility makes the first failure end cleanup. Interruption is fiber cancellation, not error handling.
⚠️ Experimental Feature: This code suggestion is automatically generated. Please review carefully.
| return Effect.interruptible(fs.remove(target, { recursive: true, force: true })); | |
| return Effect.uninterruptible(fs.remove(target, { recursive: true, force: true })); |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🤖 AI Review
Verified all five submitted findings and merged the duplicate cache finding into four entries. Confirmed the cache-refresh regression, a pre-existing test-cleanup weakness, and the test-naming nit. The secret-cleanup impact remains uncertain because the filesystem adapter implementation is unavailable. No next-context file was present. Tests were not run.
Findings
| Severity | Location | Category | Sources | Claim |
|---|---|---|---|---|
| 🟡 MINOR | apps/cli/src/command-internal/upgrade-notice.ts:175 |
compatibility |
claude+codex | Refreshing an existing writable cache now requires write permission on its parent directory. With a read-only directory, the stale cache cannot record a fetched version or offline backoff, so subsequent invocations repeat the network lookup. |
| 🟡 MINOR | apps/cli/src/command-internal/start-secrets-cleanup.ts:31 |
error-handling |
claude | A failed secret-directory removal can interrupt sibling cleanup effects and leave other containers' staged-secret directories behind. |
| ⚪ NIT | apps/cli/src/command-internal/db-bootstrap/db-setup.unit.test.ts:254 |
test-hygiene |
claude | Temporary directories remain on disk when these tests fail before reaching their trailing fs.remove call. |
| ⚪ NIT | apps/cli/src/command-internal/upgrade-notice.unit.test.ts:597 |
test-naming |
codex | Renamed tests add Go implementation comparisons instead of describing the CLI contract in its own terms. |
Stats
Claude findings: 3 · Codex findings: 2 · Confirmed: 3 · Refuted: 0 · Uncertain: 1
Models: claude-opus-5-5 + gpt-6.1-sol · Trigger: manual · Workflow run
This review runs once per PR. A maintainer can request another with a /ai-review comment.
| const writeCacheFile = Effect.fnUntraced(function* (cacheFile: string, contents: string) { | ||
| const fs = yield* FileSystem.FileSystem; | ||
| yield* fs | ||
| .remove(cacheFile) | ||
| .pipe(Effect.catchReason("PlatformError", "NotFound", () => Effect.void)); | ||
| yield* fs | ||
| .writeFileString(cacheFile, contents, { flag: "wx", mode: 0o644 }) | ||
| .pipe(Effect.catchReason("PlatformError", "AlreadyExists", () => Effect.void)); | ||
| }); |
There was a problem hiding this comment.
🟡 MINOR · compatibility · source: claude+codex
Refreshing an existing writable cache now requires write permission on its parent directory. With a read-only directory, the stale cache cannot record a fetched version or offline backoff, so subsequent invocations repeat the network lookup.
Evidence: upgrade-notice.ts:177-182 removes the cache before recreating it with flag "wx". Lines 322-331 determine freshness from its unchanged mtime and fetch whenever stale. upgrade-notice.unit.test.ts:378-390 explicitly expects EACCES and the old cache contents when the directory mode is 0555.
Suggested fix: Preserve a no-follow in-place writer for existing regular files through an Effect adapter, retaining exclusive creation for absent files and the read-only-directory regression test.
There was a problem hiding this comment.
The remove-then-wx cache writer and its read-only-directory test come from develop's effect-native upgrade-notice change, not from the merge. It is out of scope for this sync.
| return Effect.interruptible(fs.remove(target, { recursive: true, force: true })); | ||
| }, | ||
| { concurrency: "unbounded", discard: true }, | ||
| ).pipe(Effect.ignore); |
There was a problem hiding this comment.
🟡 MINOR · error-handling · source: claude
A failed secret-directory removal can interrupt sibling cleanup effects and leave other containers' staged-secret directories behind.
Evidence: start-secrets-cleanup.ts:21-34 runs interruptible fs.remove effects concurrently and ignores errors only outside Effect.forEach. The helper runs in finalizers at commands/stop/stop.handler.ts:160-162 and commands/start/start.handler.ts:1436-1438, and during rollback at db-bootstrap/rollback.ts:56.
Suggested fix: Ignore removal errors within each iteration so a failure cannot interrupt sibling cleanup effects, and remove the explicit interruptible wrapper unless required.
Adjudication (uncertain): The fail-fast effect structure and finalizer callers are verified. However, the installed filesystem adapter source is absent, so I could not establish whether interruption cancels native removals already underway or merely stops awaiting operations that continue deleting. The available tests do not exercise concurrent cleanup with a failing removal.
There was a problem hiding this comment.
The concurrent cleanup in start-secrets-cleanup.ts is existing develop code that the merge did not change. It is out of scope for this sync.
| expect(execSql.some((sql) => sql.endsWith(SCHEMA_14_FINGERPRINT_SUFFIX))).toBe(true); | ||
| expect(execSql.some((sql) => sql.includes(SCHEMA_13_FINGERPRINT))).toBe(false); | ||
| expect(docker.runs.length).toBe(0); | ||
| yield* fs.remove(workdir, { recursive: true, force: true }); |
There was a problem hiding this comment.
⚪ NIT · test-hygiene · source: claude
Temporary directories remain on disk when these tests fail before reaching their trailing fs.remove call.
Evidence: db-setup.unit.test.ts:161-164 creates an unscoped temporary directory. Lines 248-254 perform the operation and assertions before cleanup; the same pattern appears at lines 265-271 and 282-289. The afterEach at lines 236-238 only restores environment stubs.
Suggested fix: Use makeTempDirectoryScoped with a scoped test, or register cleanup through an acquisition finalizer.
There was a problem hiding this comment.
The unscoped temp directories in db-setup.unit.test.ts come from develop, not from the merge. They are out of scope for this sync.
| ctx.cleanup(); | ||
| }); | ||
| it.effect( | ||
| "--help ignores the project dotenv opt-out — Go never loads config for the built-ins", |
There was a problem hiding this comment.
⚪ NIT · test-naming · source: codex
Renamed tests add Go implementation comparisons instead of describing the CLI contract in its own terms.
Evidence: upgrade-notice.unit.test.ts:597 adds "Go never loads config for the built-ins"; lines 363 and 444 add "Go-compatible modes" and "like godotenv before the Execute tail". trusted/CLAUDE.md:115 discourages Go-parity framing, and line 118 requires test names to carry intent.
Suggested fix: Remove implementation-comparison suffixes and retain the concrete behavior or expected permission modes in each test name.
There was a problem hiding this comment.
These test names come from develop's upgrade-notice change, not from the merge. They are out of scope for this sync.
Claude repair roundNo CI check failed on this sync, so no code changes were needed. All four review findings are about code 0 findings fixed, 4 declined (workflow run). |
supabase-oss
left a comment
supabase-oss
left a comment
There was a problem hiding this comment.
CI is green and the AI review raised nothing the merge introduced, so this approves Claude's resolution of the sync (https://github.com/supabase/cli/actions/runs/38901653729).
Merging
developintonextconflicts. This branch starts at the tip ofdevelop(57f336e);nextis at3d62653.Conflicting files:
.oxlintrc.effect.jsonapps/cli/src/command-internal/db-bootstrap/db-setup.unit.test.tsapps/cli/src/command-internal/db-bootstrap/shadow-cache.integration.test.tsapps/cli/src/command-internal/db-bootstrap/start-local-database.tsapps/cli/src/command-internal/stack-storage.native.integration.test.tsapps/cli/src/command-internal/test-db.handler.tsapps/cli/src/command-internal/upgrade-notice.integration.test.tsapps/cli/src/command-internal/upgrade-notice.tsapps/cli/src/command-internal/upgrade-notice.unit.test.tsTo resolve:
git fetch origin git switch sync/develop-into-next git merge origin/next # resolve the conflicts, then commit git push origin sync/develop-into-nextApprove to fast-forward
next; do not use the merge button.