fix(cli): strict validation for [tool.fal] pyproject manifest

[burak-fal]

Summary

Fixes VUL-628 (narrowed scope, see Linear comment on the ticket — one of the
four originally-reported defects was already stale on main at the time of
verification and is not part of this PR).

Four gaps in [tool.fal] manifest parsing let misconfiguration through
silently instead of failing closed at parse time:

• Top-level [tool.fal] keys other than apps were dropped with no
  warning (fal/cli/_utils.py). Now warns (not a hard error yet — an
  existing project may already carry a stray key today; a future release
  can promote this to an error once the warning has had a chance to surface
  it).
• Key normalization (k.replace("--", "").replace("-", "_")) could collide
  two distinct top-level keys with silent last-wins semantics
  (fal/project.py). Now raises, naming both colliding keys.
• auth had no client-side validation, unlike the --auth CLI flag which
  already validates against ALIAS_AUTH_MODES. Now validated against the
  same constant, no second source of truth.
• keep_alive accepted negative integers (_validate_int only checked
  isinstance). Added _validate_non_negative_int.

Test plan

• New unit tests: tests/unit/test_project.py (normalization/collision,
  6 tests), tests/unit/cli/test_utils.py (unknown-key warning, auth
  validation, keep_alive range, full-manifest regression, 9 tests)
• Full tests/unit suite: 897 passed, 3 skipped (hardware-dependent),
  6 pre-existing failures in test_deploy.py reproduced identically on
  main before this change (path/project-root related, unrelated to
  this diff)
• ruff check / ruff format --check clean

🤖 Generated with Claude Code

---

Note

Low Risk
CLI-only manifest validation with fail-closed errors for collisions and invalid auth/keep_alive; stray top-level keys warn only, so existing manifests are unlikely to break.

Overview
Tightens [tool.fal] manifest handling so bad config surfaces at parse time instead of being ignored or accepted silently.

parse_pyproject_toml now errors when two top-level keys normalize to the same name (dash/underscore/-- stripping), instead of last-wins. get_app_data_from_toml prints a warning for unexpected top-level keys besides apps (not a hard error yet), validates auth against ALIAS_AUTH_MODES, and rejects negative keep_alive via _validate_non_negative_int. Unit tests cover collisions, warnings, auth, keep_alive, and realistic manifests.

^{Reviewed by Cursor Bugbot for commit 18d5d86. Bugbot is set up for automated code reviews on this repo. Configure here.}

[linear-code]

VUL-628

[claude]

I won't approve this run — the inline finding is a real regression: fal run now hard-fails on a toml auth value it explicitly discards in favor of the --auth flag, so a previously-working (if misconfigured) manifest now breaks a command that never uses that field. The rest of the diff (top-level key collision check, unexpected-key warning, non-negative keep_alive) is small and well-tested, but this regression is worth a human decision on whether to scope the auth check to deploy only.

Extended reasoning...

Overview

The PR adds stricter fail-closed validation to [tool.fal] pyproject manifest parsing: (1) a warning for unexpected top-level keys, (2) a hard error when two top-level keys normalize to the same name after dash/underscore stripping, (3) validation that auth is one of ALIAS_AUTH_MODES, and (4) rejection of negative keep_alive. Changes are confined to fal/cli/_utils.py and fal/project.py, with matching new unit tests.

Security risks

None of substance. This is client-side CLI manifest parsing with no auth bypass, injection, or data exposure vector. The ruled-out candidate (warning printing raw unexpected-key values) was examined and is not a real leak since it only echoes back what the user themselves wrote into their own pyproject.toml.

Level of scrutiny

This is a low-blast-radius CLI change, but it changes fail/succeed behavior for existing manifests, which warrants care. The inline finding shows one path (fal run) that previously succeeded now fails outright because the new auth validation runs unconditionally inside a function shared by both fal run and fal deploy, even though run.py explicitly discards the toml auth value moments later. That is a real, if narrow, regression — it only triggers for manifests with an already-invalid auth string, and produces a clear error message, but it is still worth a human call on whether to scope the check to deploy-only or add a run-path bypass.

Other factors

Test coverage for the new logic is solid (15 new unit tests covering warnings, auth validation, keep_alive range, and full-manifest regressions), and the PR author already scoped down from the original 4-item ticket after one issue was found stale. Given the regression is real but minor, I am deferring rather than blocking outright.

[claude]

🟡 This PR adds auth validation to get_app_data_from_toml(), which is called unconditionally by fal run even though fal run explicitly discards the toml auth value in favor of the --auth CLI flag (see the warning at run.py:33-40). As a result, a manifest with an invalid auth value (e.g. auth = "privat") that previously ran fine under fal run now hard-fails with a ValueError on a field the command never uses. Consider skipping this validation for the run path, or only enforcing it once the field is actually needed (e.g. deploy).

Extended reasoning...

The bug: get_app_data_from_toml() in projects/fal/src/fal/cli/_utils.py:112-116 now unconditionally validates the auth value parsed from pyproject.toml against ALIAS_AUTH_MODES and raises ValueError if it doesn't match. This function is shared between fal run and fal deploy, but the two commands treat the toml auth field very differently.

Why fal run is affected despite not using this field: run.py:43 calls get_app_data_from_toml(app_name) to get toml_data. Immediately after, at run.py:44-48, the result is overridden via replace(toml_data, auth=app_data.auth, name=app_data.name), where app_data.auth comes from the --auth CLI flag (default public), not from the toml. run.py:33-40 even prints an explicit warning before this happens: "fal run ignores fal.App auth and pyproject.toml auth and defaults to public." So the toml auth value is fetched and then thrown away — by design, the command's own comments/warnings describe this as intentional, unused data.

Why the new validation breaks this: The new check at _utils.py:112-116 runs inside get_app_data_from_toml(), which executes before run.py's replace() call ever has a chance to discard the toml auth. So if a manifest has an app entry with a typo'd or stale auth value (e.g. auth = "privat" instead of "private"), fal run <app> now raises ValueError: App <app> auth must be one of [...] in pyproject.toml, got 'privat'. and aborts — even though that field has zero effect on how the app is actually run.

Why nothing else in the code prevents this: There's no run-vs-deploy branch inside get_app_data_from_toml(); the function has no context about which CLI command invoked it. The only signal is the emit_deprecation_warnings kwarg, which controls whether warnings print, not whether stricter checks raise. The new auth validation is unconditional and hard-fails regardless of caller.

Step-by-step proof:

1. A developer has [tool.fal.apps.my-app] with ref = "..." and auth = "privat" (typo for "private") in pyproject.toml. This works today because fal run never validates or uses the toml auth.
2. Developer runs fal run my-app.
3. run.py:43 calls get_app_data_from_toml("my-app").
4. Inside that function, app_auth = app_data.pop("auth", None) yields "privat".
5. The new check if app_auth is not None and app_auth not in ALIAS_AUTH_MODES is true, so ValueError is raised immediately — before run.py ever reaches its replace(toml_data, auth=app_data.auth, ...) override at line 44-48.
6. fal run now fails outright, even though the printed warning literally tells the user this field is ignored and the CLI --auth flag (defaulting to public) would have been used instead.

Impact and fix: This is a real regression — a previously-working invocation of fal run now fails on a manifest field the command documents as unused. It only triggers on an already-misconfigured manifest (a typo'd auth), and the resulting error message is clear/actionable, which limits real-world impact — most users won't have an invalid auth string sitting in their manifest, and those who do will get an understandable error rather than silent misbehavior. Still, it's worth fixing: e.g. skip the toml auth validation for the run path (perhaps via an added parameter), or move the validation to only apply where the field is actually consumed (deploy).