Repository navigation
refactor(cli): resolve local config values through effect (CLI-2837) - #7301
Open
7ttp wants to merge 4 commits into
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🤖 AI Review
Verified all three Claude findings against the checked-out code and trusted conventions; all are confirmed as minor concerns. The passkey defect and direct environment access predate this migration and remain present. The symlink permission test leaves the new successful-parent-probe case unverified in Linux CI. Codex reported no findings. Verification was static; dependencies were unavailable for running tests.
Findings
| Severity | Location | Category | Sources | Claim |
|---|---|---|---|---|
| 🟡 MINOR | apps/cli/src/command-internal/stack-config.ts:1045 |
error-handling |
claude | When an auth.passkey section exists, an invalid SUPABASE_AUTH_PASSKEY_ENABLED override or unparseable raw enabled value becomes a defect through Effect.orDie instead of a typed StackConfigError. This preserves a pre-existing error-handling gap. |
| 🟡 MINOR | apps/cli/src/command-internal/local-config-values.ts:473 |
consistency |
claude | decryptAuthSecret bypasses ConfigProvider by collecting DOTENV_PRIVATE_KEY* values directly from process.env. Consequently, the new isolated ConfigProvider does not isolate encrypted-secret tests, whose helper clears only the unsuffixed DOTENV_PRIVATE_KEY. |
| 🟡 MINOR | apps/cli/src/command-internal/config-validate.unit.test.ts:270 |
test-coverage |
claude | The test for a symlink whose own permissions deny readLink returns without assertions on Linux, leaving the new PermissionDenied-with-searchable-parent case without Linux CI coverage. |
Stats
Claude findings: 3 · Codex findings: 0 · Confirmed: 3 · Refuted: 0 · Uncertain: 0
Models: claude-opus-5-5 + gpt-6.1-sol · Trigger: auto · Workflow run
This review runs once per PR. A maintainer can request another with a /ai-review comment.
This branch has not been deployed
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
local config values, config validation and path containment run through effect, so the effect lint now covers
apps/cli/srcwhats improved/fixed?
Configand files throughFileSystem, and fail with typed errors instead of throwingprocess.env, so a shell override can no longer skew themcommand-internalentries for one!apps/cli/src/**entryhow?
start,status,stop,services,notebooks,config pushandstack startyield the converted helpers and map failures with the same messages they used to catch
SIDE_EFFECTS.mdnotes now sayfs.realPathwhere they saidrealpathSyncNote
(same here) nothing changes for users,
every command prints the same output and exits with the same codes and errors.
local-db-runningstays excluded, it talks to the docker socket or named pipe over node http and needs a windows check first.ref: