Repository navigation
Conversation
There was a problem hiding this comment.
Looks good — a well-contained CLI refactor with correct enum mapping and no security-sensitive surface. Checked: args.console is always set before dispatch (fal/cli/main.py:80), so _prompt_preset referencing it is safe; KeyScope only has ADMIN/API members, so KeyPreset.from_scope's mapping is exhaustive; the interactive-prompt guard raises ValueError (rather than hanging) whenever sys.stdin.isatty() is false, matching the same isatty-mocking pattern already used for interactive prompts in test_deploy.py; and the new --preset/--scope mutual exclusivity plus preset/scope round-trip mapping are covered by the added unit tests.
Extended reasoning...
Overview
The PR adds a KeyPreset enum (FULL/API) in projects/fal/src/fal/sdk.py that maps to/from the existing KeyScope enum, and reworks fal keys create in projects/fal/src/fal/cli/keys.py to accept an optional --preset (mutually exclusive with the now-optional, deprecated --scope), an --alias alias for --desc, and an interactive preset picker (_prompt_preset) when neither flag is supplied. Tests were added covering flag parsing, mutual exclusivity, and the preset/scope mapping functions.
Security risks
None identified. No new external input handling beyond argparse choices, no crypto/auth logic changes (the actual key material generation is unchanged server-side), and the preset-to-scope mapping is a straightforward, exhaustive translation since KeyScope only has two members (ADMIN, API).
Level of scrutiny
This is a small, mechanical, additive CLI change (new optional flag plus an interactive fallback) that preserves backward compatibility with --scope. I verified the two candidate issues surfaced by the bug hunter and found both were already handled by the code: (1) the interactive prompt cannot hang non-interactive callers because it explicitly checks sys.stdin.isatty() and args.console.is_terminal and raises ValueError immediately if not a TTY, rather than blocking on Prompt.ask; (2) the same isatty-mocking approach used elsewhere in this codebase's test suite (test_deploy.py) would work to drive/test this new prompt, so it isn't uniquely untestable, even though a dedicated unit test for the interactive branch wasn't added in this PR.
Other factors
Test coverage for the new non-interactive paths (preset flag, alias, mutual exclusivity, scope/preset mapping) is present and passes review by inspection. The interactive prompt path itself has no direct unit test, which is a minor gap but not a correctness bug, and consistent with the "obvious/mechanical, no ambiguity" bar for approval.
fal keys create takes --preset FULL|API, accepts --alias alongside --desc, and prompts for a preset when neither --preset nor the now deprecated --scope is given. Presets map onto the existing scope field, so the wire format is unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Also raise a ValueError for the non-interactive preset prompt instead of SystemExit, so the message goes through the CLI's own error handling. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
b4c934d to
67b52c1
Compare
|
I think internally we're using policy_preset but exposing "preset" terminology externally is not super natural. WDYT about --access full|api ? |
"policy" is an internal term and should not surface to users; a preset is a named permission bundle. Also fixes the long-standing "privilage" typo on the --scope help line. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Going to stick with |
|
Superseded by #1185, which combines all three layers into a single PR so the CLI features ship in one release. The work from this PR is carried over there unchanged apart from the preset no longer being sent on the deprecated |
fal keys createrequires--scope ADMIN|API, a binary choice that cannot express anything narrower. This moves the CLI to named permission presets.fal keys createnow takes--preset FULL|APIand accepts--aliasalongside--desc. With neither--presetnor the now-deprecated--scope, it prompts for a preset in a TTY; a non-interactive run has to pass a flag. Presets map onto the existing scope field — FULL to ADMIN, API to API — so the wire format is unchanged and this stands alone.Presets with no scope equivalent, and custom permission lists, need a wire field and follow in #1179 and #1180.
Note
Medium Risk
Changes how API keys are created in the CLI (permission selection and interactive defaults); wire format is unchanged but mis-specified presets or CI scripts missing flags could behave differently than before.
Overview
fal keys createnow centers on permission presets (FULL/API) instead of requiring--scope. Users can pass--preset, keep--scopeas a deprecated alias (mapped viaKeyPreset.from_scope), or pick a preset from an interactive Rich table when running in a TTY; non-interactive runs must pass--presetor--scope.--aliasis added as a synonym for--desc, and success output refers to the preset name.A new
KeyPresetenum in the SDK defines preset ↔KeyScopemapping (FULL→ADMIN,API→API) so key creation still uses the existing scope wire field unchanged.Unit tests cover
--preset/--aliasparsing, rejecting--presetand--scopetogether, and the preset/scope mapping helpers.Reviewed by Cursor Bugbot for commit c025ddf. Bugbot is set up for automated code reviews on this repo. Configure here.