The flag surface, judged as a set: `--address` means two things, the server's listen address has no flag, and one variable is both a socket and a URL
Nobody has claimed this yet.
- Dominant language
- Go
- Stars
- 9
- Forks
- 0
- Avg merge
- 3h 3m
- Merged PRs (30d)
- 509
Description
flow's flags were each designed well and never judged as a set. Read together they carry one spelling with two meanings, one meaning with five spellings, and one environment variable that is simultaneously a socket to bind and a URL to dial — which is not a style complaint, because #569's review found the point where that last one stops being survivable. This is the audit and the cleanup it implies. Serving-surface design hangs off #549 and is sequenced by #567; this issue is the flag surface those slices are about to add to, and it is worth settling the naming rule before ACME and mTLS put another dozen flags through it.
Verified against origin/main at write time, plus the open PR #569 where noted. Line numbers are main's unless the text says otherwise.
The defects
1. --address means two different things, decided by which command declares it. cmd/flow/main.go:1488 registers it on serverCmd and workerCmd as "Temporal server address (overrides environment configuration)". cmd/flow/client.go:93 registers it on fifteen client verbs as "address of the Flowstate server". docs/reference/cli.md renders both — --address appears in thirteen tables meaning the Flowstate server and once, at line 1064, meaning Temporal's. This is known: cmd/flow/serverdev.go:161-167 names the ambiguity in a comment and dodges it by spelling its own flag --listen, and docs/DEPLOYMENT.md:436 calls it "a real foot-gun on a platform that hands you a --address-shaped port variable and expects it to mean 'listen here.'" Documented twice, fixed nowhere.
2. flow server has no flag for the address it listens on. cmd/flow/main.go:761 reads os.Getenv("FLOWSTATE_ADDRESS") inline at the point of constructing the http.Server. It is the only setting in the tree that is environment-only, and it is the one setting an operator most reliably needs to change: docs/DEPLOYMENT.md:429-435 documents the workaround for Cloud Run and fly.io as an entrypoint script translating $PORT into FLOWSTATE_ADDRESS. #569 inherits the shape — its publicAddr is still cmp.Or(os.Getenv("FLOWSTATE_ADDRESS"), defaultServerAddress), computed a few lines above four brand-new flags on the same command.
3. One variable is a bind address and a client URL. cmd/flow/internal/docsgen/envvars.go:59-63 documents FLOWSTATE_ADDRESS in a single sentence — "Address the API server listens on, and that the client commands connect to." Client-side, serverBaseURL (cmd/flow/client.go:202-212) honors an explicit http:// or https:// and prefixes http:// when there is none. Server-side the same string goes to net.Listen, which has no use for a scheme at all. The two readings are compatible today only by coincidence: localhost:9233 happens to be legal as both a bind address and a URL authority, and plaintext happens to be what the server serves. #569's P2 review comment is where the coincidence runs out — with TLS configured, https://flowstate.example.com breaks net.Listen and flowstate.example.com:443 makes the client speak http:// to a TLS socket. There is no value of the variable that works.
4. Three components already hold three different positions on whether this deployment speaks https. The client refuses to put a bearer token on a non-loopback plaintext connection (cmd/flow/credentials.go:145-158). The trust policy refuses a federation.issuer that is not https:// unless it is loopback (pkg/flowstate/v1/auth/policy.go:459 through validateHTTPSURL at :478) — so any deployment that configures federation has already written down https://flowstate.example.com as its own name. And cmd/flow/main.go:761 serves the discovery document and JWKS at that exact origin over plain HTTP. The policy demands https, the client refuses http, the server offers only http. #569 fixes the server's half; nothing yet connects the certificate's hostname to the issuer URL that names the same host, which is the "one value, written down twice" shape CLAUDE.md is about.
5. namespace is five things. --namespace on flow server and flow worker (cmd/flow/main.go:1489) is Temporal's namespace. --tenant on flow worker (:1509) is the Flowstate tenant. --webhook-namespace on flow server is also the Flowstate tenant. --as-namespace is the tenant of a simulated principal. --secret-vault-namespace is Vault's own namespace, and its help text has to spend a clause disowning the others: "this is the vault's own namespace, not the tenant namespace a run authenticates with". #568 is this same collision one layer down, in the CEL vocabulary.
6. --secret-env-namespace is a map where its four siblings are booleans. --secret-dir-namespaced, --secret-keychain-namespaced, --secret-op-namespaced and --secret-command-namespaced are all bool. --secret-env-namespace is a repeatable NAMESPACE=PREFIX list. One letter apart, different type, different question answered. The fail-closed switch for the whole family is a sixth flag with a seventh spelling, --secret-require-namespace, defaulting to false — so the posture CLAUDE.md's "Test that A cannot reach B" section was written about is reachable, but only by an operator who finds six flags and knows they are one decision.
7. "Accept this exposure by name" is a real convention with three spellings. --insecure-no-auth, --allow-insecure-plugin-dir, --allow-unversioned-interpreter, FLOWSTATE_INSECURE_PLAINTEXT_TOKEN. The pattern is one the repo believes in and states well; nothing makes the next one land on the same prefix.
8. The read: column of docs/reference/envvars.md is unchecked prose, and three entries are wrong. envvars.go:62 attributes FLOWSTATE_ADDRESS to cmd/flow/client.go, cmd/flow/main.go; cmd/flow/serverdev.go:169 and :325 read it too. envvars.go:80 attributes FLOWSTATE_AUTH_POLICY to cmd/flow/main.go, cmd/flow/mcp.go; serverdev.go:187, serverdev.go:373 and taskrun.go:206 read it. envvars.go:140 says the same for FLOWSTATE_IDENTITY_KEY; serverdev.go:192 and taskrun.go:208 read it. The struct's own comment says the column exists "so a reader who doubts the sentence can go and check it" (envvars.go:20-21), and TestEveryEnvironmentReadIsDocumented already collects every call site into found.names[name] — it compares the name set in both directions and discards the locations it gathered on the way. The gate has the data and does not look at it.
9. #569's plaintext refusal makes two shipped deployments unstartable, and only one has been reported. Codex's P1 on that PR names examples/observability/docker-compose.yaml:72, which sets FLOWSTATE_ADDRESS: 0.0.0.0:9233 with no TLS flags. docs/DEPLOYMENT.md:414 has the same defect and nobody has flagged it: the documented Kubernetes recipe is flow server behind an Ingress doing TLS termination, which means the pod binds 0.0.0.0 in plaintext deliberately and correctly. Terminating TLS in front is not a workaround there, it is what docs/DEPLOYMENT.md:490-496 tells every reader to do. The client already shows the shape of the answer — FLOWSTATE_INSECURE_PLAINTEXT_TOKEN exists, in the words of cmd/flow/credentials.go, "for the person terminating TLS at a sidecar who knows what their network is; it is named so that finding it in a shell profile is alarming." The server needs the same door with the same volume. This one belongs to #569's review rather than to this issue, and is recorded here because it is a flag-naming decision either way.
Out of scope, already decided: the internal listener's default. The owner's decision on #569 is that the flag's zero value means no internal listener and 127.0.0.1:9090 is the canonical example rather than the default.
The rule
One sentence, and its edges matter as much as its middle: a flag names the noun it belongs to. Temporal's settings carry --temporal-. A socket this process binds is --listen. A place this process dials is --address. namespace means a Flowstate tenant, or it is retired.
Where the rule stops: --task-queue and --task-queue-prefix stay unprefixed. A task queue is a Temporal noun the way a "run" is, but which queue this deployment uses is a Flowstate decision — --task-queue-prefix composes <prefix>_<namespace> from our tenant, and prefixing it --temporal- would say Temporal chose it. A rule that cannot say where it stops is a preference.
Before and after
Illustrative spellings, not the landed shape.
| today | means | after |
|---|---|---|
flow server --address, flow worker --address |
Temporal endpoint | --temporal-address |
flow server --namespace, flow worker --namespace |
Temporal namespace | --temporal-namespace |
flow server --profile, flow worker --profile |
Temporal profile | --temporal-profile |
FLOWSTATE_ADDRESS read at main.go:761, no flag |
the socket flow server binds |
flow server --listen, FLOWSTATE_LISTEN |
flow server dev --listen |
the socket it binds | unchanged — it was right first |
client --address / FLOWSTATE_ADDRESS |
endpoint to dial | spelling unchanged, meaning narrowed to a URL |
flow worker --tenant |
Flowstate tenant | --tenant everywhere |
flow server --webhook-namespace |
Flowstate tenant | --webhook-tenant |
--secret-env-namespace |
tenant→prefix map | --secret-env-namespaced (bool) + --secret-env-prefix (the map) |
--allow-insecure-plugin-dir |
accept an exposure | --insecure-plugin-dir |
--allow-unversioned-interpreter |
accept an exposure | --insecure-unversioned-interpreter |
The one entry that is deliberately not a reuse: after --temporal-namespace frees the name, --namespace is retired rather than repurposed to mean the tenant. #565 and #567 already paid for this lesson — one spelling that quietly changes which principal it names is how a deny rule stops matching without warning, and a flag whose meaning changed between releases is the same defect with a shell script instead of a policy file. --tenant is the surviving spelling because it was never ambiguous.
Migration is MarkDeprecated for everything above, with one exception. --address on flow server and flow worker cannot merely warn: continuing to mean Temporal while the reader believes it means "listen here" is precisely the foot-gun DEPLOYMENT.md already documents, and the failure is silent — the server comes up on the default port and nothing says why. It should become an error naming both replacements. Likewise FLOWSTATE_ADDRESS on flow server: accept it for one release with a deprecation line, refuse it outright if it carries a scheme, since a scheme can only have been meant for the client.
The types that make defect 3 unrepresentable
Renaming the flags fixes the human confusion. Two types fix the machine's, and they are what keeps #569's P2 from coming back the next time somebody adds a listener.
// ListenAddress is a socket this process binds. Never a URL: a scheme is a
// client's fact about how to reach us, and net.Listen has no use for one.
// Parsed once at startup, so "0.0.0.0" and ":9233" and "[::1]:0" are all
// answered here rather than at four call sites.
type ListenAddress struct{ hostPort string }
func ParseListenAddress(s string) (ListenAddress, error) // refuses a scheme
func (a ListenAddress) Listen() (net.Listener, error)
func (a ListenAddress) IsLoopback() bool
// ServerEndpoint is where a client sends requests. It always carries a
// scheme after parsing, because "is this connection plaintext" is a question
// credentials.go must answer without guessing, and prefixing "http://" onto
// a bare string is a guess.
type ServerEndpoint struct{ url *url.URL }
func ParseServerEndpoint(s string) (ServerEndpoint, error) // bare host:port -> http://, warned
func (e ServerEndpoint) IsPlaintext() bool
Neither converts to the other, so net.Listen(endpoint) does not compile and serverBaseURL(listenAddr) does not compile. isLoopbackAddress (cmd/flow/client.go:265) currently strips a scheme by hand precisely because it is asked both questions by both kinds of caller; with two types it becomes two methods that cannot drift.
The gate that keeps it
documentedEnvironmentVariables in cmd/flow/internal/docsgen/envvars.go is the prior art, and it is the right shape: a hand-kept table plus a drift test in both directions, because there is no registration point to derive from. The flag surface does have a registration point — the cobra tree cmd/flow/internal/docsgen/cli.go already walks — so the check is cheaper than the env-var one:
// Illustrative. One entry per flag name that appears on more than one
// command, saying the single thing it means. A name declared twice with two
// meanings has to be spelled twice.
var flagMeanings = map[string]string{
"listen": "a socket this process binds",
"address": "the Flowstate server a client dials",
"temporal-address": "the Temporal frontend this process dials",
"tenant": "a Flowstate tenant namespace",
}
// TestNoFlagNameCarriesTwoMeanings walks the cobra tree and fails on a name
// whose usage strings disagree about the noun. The failure message is the
// table entry, so the fix is either a rename or a deliberate edit here.
And the cheap half of defect 8, with data the test already holds:
// In TestEveryEnvironmentReadIsDocumented, alongside the two set checks:
// the sites are gathered and then thrown away, which is why three entries
// name two files each and read four.
assert.ElementsMatch(t, found.names[name], strings.Split(entry.read, ", "),
"%s is documented as read in %s and is actually read in %s", ...)
Acceptance
A reader of docs/reference/cli.md can tell what a flag does from its name without knowing which command declared it. flow server --listen :443 works and FLOWSTATE_ADDRESS no longer decides where anything binds. net.Listen cannot be handed a URL and a client cannot be handed a bind address, because the types differ. Every deprecated spelling either warns or refuses, and the ambiguous one refuses. docs/reference/envvars.md's read: column is checked by the test that already collects it.
Questions
- Scope: one cleanup PR for the whole table, or the address flags now (they block #549's remaining slices) and the
namespace/secret/insecurerenames as a follow-up? Recommended: split, address flags first, because ACME and mTLS both add flags to exactly that group and doing it after means renaming them twice. --addressonflow server/flow worker: error, or deprecate-and-warn for one release? Recommended error, because the failure it prevents is silent.FLOWSTATE_ADDRESSonflow server: keep it working as a deprecated alias forFLOWSTATE_LISTENfor one release, or cut it immediately? Recommended: keep for one release, refuse it if it carries a scheme.- The two types:
cmd/flowinternal, orpkg/flowstate/v1/…where a future protoListenermessage would land? Recommendedcmd/flowfor now — #549's constraint says the serving configuration eventually becomes a schema, and a Go type that exists to be un-serializable should not pre-empt that message. - Defect 9 (
refusePlaintextListenerversus the documented Ingress recipe) belongs to #569's review, not here — but the escape hatch needs a name. Recommended--insecure-plaintext-listener/FLOWSTATE_INSECURE_PLAINTEXT_LISTENER, matchingFLOWSTATE_INSECURE_PLAINTEXT_TOKENon the client, which is the same operator making the same statement about the same network.
Generated by Claude Code
Generated by Claude Code
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start with the cited flag registrations in cmd/flow/main.go and cmd/flow/client.go, then inspect cmd/flow/internal/docsgen/envvars.go and TestEveryEnvironmentReadIsDocumented. Review #569 alongside the deployment examples and docs to define the migration and listener/client-address boundaries; done means the naming rule, address types, affected documentation, and coverage are settled consistently.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- go
- Domain
- backend, cli
- Issue type
- Refactor
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Quiet
- Clarity
- Mostly clear
- Newbie friendliness
- 35/100