Repository navigation
feat(cli): experimental compute - add secrets scope field - #7531
kallebysantos wants to merge 32 commits into
Conversation
e66cc7f to
f2c631c
Compare
CLI benchmark (Lightweight Commands): no changes foundCommit
API calls: unchanged for all 7 commands. Binary size: 134.9 MB, up 12.0 KB from 134.9 MB. Details: timing ranges, API calls per command, slowest phasesTiming ranges
API calls per command
Slowest phasesNo command moved beyond noise, so there are no phase deltas. Comment |
There was a problem hiding this comment.
🤖 AI Review
Reconciled all 13 Claude and 6 Codex findings into 13 entries. Confirmed incomplete flag metadata, blank custom secret names, missing override warnings, incorrect error classification, stale documentation, and cleanup issues. Refuted the malformed-config exposure claim, the schema-enum requirement, and the redundant-test allegation. Verification was static; dependencies were unavailable for executing checks.
Findings
| Severity | Location | Category | Sources | Claim |
|---|---|---|---|---|
| 🟡 MINOR | apps/cli/src/shared/compute/compute-runtimes.ts:169 |
input-validation |
claude+codex | Custom scope parsing accepts blank secret names and carries them into configuration and deployment requests. |
| 🟡 MINOR | apps/cli/src/commands/experimental/compute/push/push.command.ts:51 |
flag-registration |
claude+codex | The new value-consuming --secrets-scope:custom spelling is missing from shared argv metadata and both commands' permission declarations. |
| 🟡 MINOR | apps/cli/src/commands/experimental/compute/push/push.handler.ts:218 |
error-handling |
claude | An invalid recorded secrets scope is reported as UnknownComputeRuntimeError, giving it an unrelated JSON error tag and telemetry fingerprint. |
| 🟡 MINOR | apps/cli/src/commands/experimental/compute/push/push.handler.ts:203 |
consistency |
claude | A restrictive secrets-scope override applies silently to one deploy, and a subsequent bare push returns to the recorded scope or all without the warning provided for exposure overrides. |
| 🟡 MINOR | apps/cli/src/commands/experimental/compute/push/SIDE_EFFECTS.md:23 |
documentation |
claude | Both command side-effect contracts omit the newly read, prompted, persisted, and deployed secrets_scope behavior. |
| ⚪ NIT | apps/cli/src/shared/compute/compute-api.ts:89 |
duplication |
claude | The built-in secrets-scope literals are duplicated, and the API converter unnecessarily revalidates already resolved values with a possible undefined result. |
| ⚪ NIT | apps/cli/src/commands/experimental/compute/push/push.handler.ts:465 |
formatting |
claude | The added code and unrelated reindentation do not follow the repository's formatter style. |
| ⚪ NIT | apps/cli/src/commands/experimental/compute/push/push.integration.test.ts:40 |
unused-import |
claude+codex | The new SupabaseApiInputError import is unused; the added tests also contain missing semicolons and misspelled override descriptions. |
| ⚪ NIT | apps/cli/src/commands/experimental/compute/new/new.integration.test.ts:359 |
misleading-comment |
claude+codex | The copied test comment incorrectly says secrets scope has no prompt and is omitted when unspecified; nearby comments and JSDoc are incomplete. |
| ⚪ NIT | packages/config/src/compute.ts:50 |
copy |
claude+codex | The public schema description and prompt hint contain the grammatical error complete hide, with additional inconsistent prompt punctuation. |
Findings outside the diff
- 🟡 MINOR
apps/cli/src/commands/experimental/compute/push/SIDE_EFFECTS.md:23— Both command side-effect contracts omit the newly read, prompted, persisted, and deployed secrets_scope behavior.
Refuted findings (kept for transparency, not posted as review comments)
apps/cli/src/shared/compute/compute-config.ts:145(security): Malformed secrets_scope values silently become undefined and cause push to expose all secrets.
Refuted: The production path validates configuration before this reader runs. cli-config-values.layer.ts:550-583 decodes the declared and materialized documents; packages/config/src/io.ts:280-299 calls validateCliConfig, whose validate.ts:6,20 uses Schema.decodeUnknownEffect(CliConfigSchema). Numbers, mixed arrays, and tables fail that schema. push.handler.ts:617 propagates the load failure rather than deploying with defaults.packages/config/src/compute.ts:47(config-schema): The secrets_scope schema should use literal enums, as exposure and size do, to reject values such as meow and ALL with trailing whitespace.
Refuted: The claimed comparison is contradicted by both the checked-out code and trusted/packages/config/src/compute.ts:22-35: size and exposure are unconstrained strings. Scope validation occurs in push.handler.ts:216-221, rejecting meow before deployment, while compute-runtimes.ts:164-166 intentionally normalizes casing and whitespace, making ALL with trailing whitespace valid. The genuine wording typo is preserved in claude-13.apps/cli/src/commands/experimental/compute/new/new.integration.test.ts:346(test-quality): The default-scope persistence test duplicates the existing full-configuration assertion and should be removed.
Refuted: These tests exercise different resolution paths. setupCompute at tests/helpers/compute.ts:506-523 makes text setup interactive and JSON setup noninteractive. new.handler.ts:221-245 selects through a prompt in the first case but returns the default without prompting in the second. The shared expected value does not make those paths redundant.
Stats
Claude findings: 13 · Codex findings: 6 · Confirmed: 10 · Refuted: 3 · 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.
| export function parseSecretsScopeCustom(secretsNames: string) { | ||
| if (typeof secretsNames !== 'string') { | ||
| return [] | ||
| } | ||
|
|
||
| return secretsNames.split(',').map(secretName => secretName.trim()) |
There was a problem hiding this comment.
🟡 MINOR · input-validation · source: claude+codex
Custom scope parsing accepts blank secret names and carries them into configuration and deployment requests.
Evidence: compute-runtimes.ts:174 splits and trims without rejecting empty elements; lines 155-166 accept every string array. new.handler.ts:235-242 supplies no prompt validator, and compute-api.ts:108-109 maps blank elements to {secret_name: ""}.
Suggested fix: Validate custom input in both flag parsing and the prompt, rejecting blank names and explaining how to select none.
| secretsScope: Flag.orElse( | ||
| Flag.Literals("secrets-scope", COMPUTE_SECRETS_SCOPE), | ||
| () => Flag.String("secrets-scope:custom").pipe(Flag.map(parseSecretsScopeCustom), Flag.withMetavar("SECRET_NAMES"))) | ||
| .pipe( | ||
| Flag.withDescription( | ||
| "Controls whether secrets are available, overriding `secrets_scope` in the project config file for this deploy. Falls back to the recorded value, then all.", | ||
| ), | ||
| Flag.optional, | ||
| ), |
There was a problem hiding this comment.
🟡 MINOR · flag-registration · source: claude+codex
The new value-consuming --secrets-scope:custom spelling is missing from shared argv metadata and both commands' permission declarations.
Evidence: push.command.ts:53 and new.command.ts:50 declare the custom flag. value-consuming-flags.ts:77 registers only secrets-scope; push.command.ts:110 and new.command.ts:121 also list only secrets-scope. value-consuming-flags.unit.test.ts:66 excludes colons from its name regex.
Suggested fix: Register the custom spelling in the value-consuming set and both permission declarations, widen the coverage regex, and verify both alternatives through the real parser and introspection helpers.
| const COMPUTE_SECRETS_SCOPE = ["all", "supabase", "none"] as const; | ||
| type ComputeSecretsScope = (typeof COMPUTE_SECRETS_SCOPE)[number]; | ||
|
|
||
| /** The `spec` a deploy sends. Mirrors the API's own field names exactly. */ | ||
| export interface ComputeDeploySpec { | ||
| readonly runtime?: string; | ||
| readonly size: string; | ||
| readonly exposure: string; | ||
| readonly secrets_scope?: ComputeSecretsScope | { | ||
| readonly secret_name: string; | ||
| }[]; | ||
| readonly instances: number; | ||
| } | ||
|
|
||
| export function parseComputeSecretsScopeToAPIFormat(value: string | string[]): ComputeDeploySpec['secrets_scope'] | undefined { | ||
| if (typeof value === 'string' && COMPUTE_SECRETS_SCOPE.includes(value as ComputeSecretsScope)) { | ||
| return value as ComputeSecretsScope; | ||
| } | ||
|
|
||
| if (Array.isArray(value) && value.every(i => typeof i === 'string')) { | ||
| return value.map((secret_name) => ({ secret_name })) | ||
| } | ||
| } |
There was a problem hiding this comment.
⚪ NIT · duplication · source: claude
The built-in secrets-scope literals are duplicated, and the API converter unnecessarily revalidates already resolved values with a possible undefined result.
Evidence: compute-api.ts:89-90 duplicates compute-runtimes.ts:135. Its converter at lines 103-111 accepts broad strings and can fall through, while push.handler.ts:364-369 resolves a ComputeSecretsScope before calling it at line 441.
Suggested fix: Share the built-in literals and make the converter total over the resolved scope type, preserving the distinct API representation for custom names.
| schedule: input.pollSchedule, | ||
| retrySchedule: input.pollRetrySchedule, | ||
| refSuffix: input.refSuffix, | ||
| onPoll: (polled) => | ||
| polled.buildState === "building" | ||
| ? deploying.message("Building compute...") | ||
| : Effect.void, | ||
| }).pipe(Effect.tapError(() => deploying.fail())); |
There was a problem hiding this comment.
⚪ NIT · formatting · source: claude
The added code and unrelated reindentation do not follow the repository's formatter style.
Evidence: push.handler.ts:214,369 omits semicolons and lines 465-472 change continuation indentation. compute-api.ts:103-109 and compute-runtimes.ts:155-178 introduce single quotes and missing semicolons. Similar reindentation appears in new.handler.ts:338-351 and new.integration.test.ts:937-938.
Suggested fix: Run the root fmt:fix script and retain only intentional changes.
7ttp
left a comment
There was a problem hiding this comment.
was having a look at it, and stumbled upon couple of things that could be improved
leaving them in-line:
| const settledSecretsScope = | ||
| typeof settled.spec.secrets_scope === "string" | ||
| ? (settled.spec.secrets_scope as string) | ||
| : settled.spec.secrets_scope?.map(({ secret_name }) => secret_name); |
There was a problem hiding this comment.
nit: this only lives in the text branch, so push -o json has exposure but no secrets_scope, while new -o json has both.
could hoist it above the if and spread it into the returned payload, like url? 🤔
| "Which secrets should be available for this compute?", | ||
| defaultFirst( | ||
| [...COMPUTE_SECRETS_SCOPE_ENUM], | ||
| DEFAULT_COMPUTE_SECRETS_SCOPE as ComputeSecretsScopeEnum, |
There was a problem hiding this comment.
maybe for later: a few new as casts (here, L237, push.handler L217, L238 and L532, compute-api L108-L109).
typing ComputeSecretsScope with ReadonlyArray<string> and the default as the literal "all" should let all of them go.
jgoux
left a comment
There was a problem hiding this comment.
A few issues to fix before merge; the first one fails the unit suite.
This validation happens on Command level
2809b88 to
361fc79
Compare
Towards FUNC-954