feat: leverage reusable cli standard options, add gprc max recv message size

[fernandezcuesta]

Description of your changes

Add support for maximum recv gprc message size.
Add "standard" CLI arguments and environment variables, matching function-sdk-go and function-sdk-python.

Fixes #39

I have:

• Read and followed Crossplane's contribution process.
• Added or updated unit tests for my change.

[stevendborrelli]

@fernandezcuesta are you still working on this?

[fernandezcuesta]

Yeah sorry for the delay. Just added also the max send option, which falls back to the same value as max-recv (and add an alias for the latter), similar as what is suggested for sdk-python and sdk-go so far.

[stevendborrelli]

Thanks for this — the CLI class is a genuinely useful addition and the test coverage on it is good. Typecheck and the full suite pass on the branch.

I verified each point below by building the branch and running it rather than by reading alone; the reproductions are in the inline comments.

The one blocking issue: serve() needs to be built on CLI, not left beside it.

The new flag table is in USAGE.md's serve() section, but serve()'s parser was never extended — the only serve.ts change here is passing message sizes through to newGrpcServer. So:

$ node main.mjs --max-recv-message-size 16 --insecure
main.mjs: Unknown option '--max-recv-message-size'

and ADDRESS=... is silently ignored.

I don't think scoping the docs to CLI is the right fix, because #39 asks for more than a reusable class:

Standardization of CLI arguments/env variables might become useful for 3rd party tooling (e.g. crossplane-diff) which may need to flag all functions in the composition pipeline at the same time (e.g. programmatically raise the max GRPC size in all functions).

That only works if every function honors the flags. The overwhelming majority are a bare serve(compose); if only functions that opt into CLI respond, a tool raising the message size across a pipeline hits functions that reject the flag outright — and the author of each one has to be persuaded to migrate. Leaving serve.ts with its own flags/descriptions/parseArgs/helpText also leaves exactly the second source of truth the issue opens by complaining about.

Concretely: have serve() construct a CLI internally and drop serve.ts's private flags and descriptions tables, with the exported parseArgs and helpText delegating to it so the public API is preserved.

One thing worth deciding deliberately as part of that: it makes the standard env vars live for every existing function. A cluster that already sets DEBUG or ADDRESS in a function's pod spec for unrelated reasons would start picking them up on upgrade. That's the intent of #39, but it's a behavioural change to call out in the release notes rather than let people discover.

The remaining findings are smaller, but four share a shape worth naming: a bad or unexpected value is silently swallowed rather than reported. A mistyped size becomes the default, a negative size reaches grpc-js unchecked, a colliding short alias is quietly dropped, and standardOptions() before parse() returns undefineds. Each is cheap to fix with an explicit validation error.

Happy to help with any of this if useful.

[stevendborrelli]

This table is under "Every function served this way accepts the same flags" in the serve() section, but serve() accepts neither --max-recv-message-size nor any environment variable.

serve.ts's flags table (serve.ts:137) still covers only address/debug/insecure/tls-server-certs-dir/help, and parseArgs reads no environment at all. Verified against the built branch:

$ node main.mjs --max-recv-message-size 16 --insecure
main.mjs: Unknown option '--max-recv-message-size'
Try 'main.mjs --help' for the available flags.

$ ADDRESS=127.0.0.1:19999 node -e "... parseArgs(['--insecure'])"
{"address":"0.0.0.0:9443", ...}   # env ignored

Rather than scoping this table to CLI, I'd suggest building serve() on CLI so the table becomes true — see the main review comment for why #39 needs that to reach every function, not just those that opt in.

Same overclaim in README's new "Custom Flags" section and at USAGE.md:194 — "the same standard flags that serve() uses". Once serve() is backed by CLI that sentence becomes accurate; today CLI is a strict superset.

[stevendborrelli]

Truthiness here drops a 0 limit and lets a negative one through.

--max-recv-message-size=-1 (or MAX_RECV_MESSAGE_SIZE=-1) produces -1048576 from standardOptions(), which lands in grpc.max_receive_message_length unchecked. That is not the -1 sentinel grpc-js reads as unlimited — it's a negative byte count.

--max-recv-message-size 0 produces 0, which this check then skips entirely, silently applying grpc-js's own 4MB default rather than whatever the operator meant by zero.

Suggest if (opts?.maxRecvMessageSize !== undefined) here, with range validation in standardOptions() so a nonsensical value is rejected at parse time instead of reaching the server.

(Minor: the bare --max-recv-message-size -1 form throws from node's parseArgs as ambiguous, so the negative path is reachable only via --flag=-1 or the env var.)

[stevendborrelli]

A malformed value is silently swallowed rather than reported. Verified:

--max-recv-message-size eight   ->  4194304   # the 4MB default
MAX_RECV_MESSAGE_SIZE=16MB      ->  16777216  # parseInt stops at 'MB', reads 16

An operator raising the limit to handle large requests who mistypes the unit keeps hitting RESOURCE_EXHAUSTED, with nothing in the logs explaining why — the flag looks accepted.

Worth throwing a usage error on non-numeric or non-positive input instead of falling back to the default.

[stevendborrelli]

Calling standardOptions() before parse() returns a config of undefineds rather than failing loudly:

new CLI({name:'f'}).standardOptions()
-> {"maxRecvMessageSize":4194304}    # address, debug, insecure, tlsServerCertsDir all undefined

That matters because serve() merges { ...parsed, ...opts.serverOptions }, so these explicit undefineds override serve's own parsed defaults, and startServer reaches server.bindAsync(undefined, ...) (runtime.ts:174). Someone copying the docs example who forgets cli.parse() gets an opaque throw from grpc-js.

Either throw a "parse() must be called first" error here, or parse lazily on first access.

[stevendborrelli]

This checks flag names against standardFlags but never spec.short, so a short-alias collision passes construction silently.

Registering verbose: { type: 'boolean', short: 'd' } constructs fine, and then:

parse(['-d'])  ->  { debug: true, verbose: undefined }

The standard flag wins and the custom flag's short is silently dead — the author's -d never reaches verbose, with no diagnostic at registration or at parse time.

Worth validating spec.short against the standard flags' shorts alongside the existing name check.

[stevendborrelli]

Forcing default = false for every boolean means a flag declared with default: true can never be turned off from the command line.

resolveValue treats "not true" as "not given on the CLI" (cli.ts:228), so an explicit false is indistinguishable from absent and falls through to env, then to spec.default. Verified: a flag registered as { type: 'boolean', default: true } resolves to true with no args, and --no-feature fails with Unknown option '--no-feature'. The only escape is an env var, and only if the spec happens to declare one.

Either reject boolean specs with a true default at construction, or support --no-<flag>.

[stevendborrelli]

DEFAULT_ADDRESS and DEFAULT_TLS_SERVER_CERTS_DIR are already exported from src/serve/serve.ts as public constants — worth importing them rather than redeclaring.

As written, changing the default listen address or certs directory in serve.ts leaves the two entrypoints binding different addresses, which is the drift the "single source of truth" comment above serve.ts's flags table was written to prevent — and the drift #39 is about.

If serve() ends up built on CLI, this resolves naturally in the other direction: the constants live here and serve.ts imports them.