Fail a bare invocation when stdout is not a terminal #4

Merged

benvin merged 10 commits from benvin/fix-invocation-guards into master 2026-09-24 21:14:38 +10:00

Conversation 2 Commits 10 Files Changed 13

+570 -26

unkin-agent commented 2026-09-22 22:50:15 +10:00

Member

Copy Link

Copy Source

Puppet's generate() feeds this binary's stdout into parsejson(), so an argv that lost its common name printed help and exited zero, feeding help text into the catalog instead of failing the compile.

• Print help on a bare invocation only when stdout is a terminal; every other fd 1 exits non-zero with empty stdout.
• Detect the terminal with a term.IsTerminal ioctl, so /dev/null and the one the runtime reopens onto a closed fd 1 are not mistaken for one.
• Reject --config "" rather than resolve it, and ignore a relative XDG_CONFIG_HOME as the spec requires.
• Pin x/term v0.45.0 and x/sys v0.47.0, the newest pair still declaring go 1.25.0.
• Drive the built binary once per kind of fd 1, pty included, and guard the tests' hermetic subprocess env outside the build tag.

Puppet's `generate()` feeds this binary's stdout into `parsejson()`, so an argv that lost its common name printed help and exited zero, feeding help text into the catalog instead of failing the compile. - Print help on a bare invocation only when stdout is a terminal; every other fd 1 exits non-zero with empty stdout. - Detect the terminal with a `term.IsTerminal` ioctl, so `/dev/null` and the one the runtime reopens onto a closed fd 1 are not mistaken for one. - Reject `--config ""` rather than resolve it, and ignore a relative `XDG_CONFIG_HOME` as the spec requires. - Pin `x/term` v0.45.0 and `x/sys` v0.47.0, the newest pair still declaring `go 1.25.0`. - Drive the built binary once per kind of fd 1, pty included, and guard the tests' hermetic subprocess env outside the build tag.

unkin-agent added 1 commit 2026-09-22 22:50:15 +10:00

Fail a bare invocation when stdout is not a terminal ... 4ff55f052a

Puppet's generate() reads stdout through a pipe with combine: true, so help
text on a zero exit lands in the catalog. Help now requires an interactive
stdout; a piped one exits non-zero with nothing on stdout.

- gate the no-args help path on an isTerminal(stdout) check
- reject an explicitly empty --config instead of resolving it
- ignore a relative XDG_CONFIG_HOME, per the XDG spec
- give the integration subprocess a hermetic HOME/XDG_CONFIG_HOME
- pass []string{} to run(), so cobra cannot fall back to os.Args

unkin-agent added 1 commit 2026-09-22 22:58:54 +10:00

Document the interactive seam in AGENTS.md 5a7fea56e2

unkin-agent commented 2026-09-22 23:07:38 +10:00

Author

Member

Copy Link

Copy Source

• cmd/certmanager/main.go:53 — ModeCharDevice is set for /dev/null, so certmanager > /dev/null prints help and exits 0; a bare call with fd 1 closed does the same (the Go runtime reopens 0/1/2 onto /dev/null). Both contradict README's "prints help and exits zero only when stdout is a terminal" → detect with a tty ioctl (golang.org/x/term.IsTerminal(int(f.Fd()))), not a stat mode bit.
• cmd/certmanager/main_test.go:851 — /dev/tty open fails in a TTY-less CI container, so TestIsTerminal skips there, and every other test injects interactive through runWith; hardwiring isTerminal to return false leaves go test ./... entirely green. The true branch is unpinned in CI → open /dev/ptmx instead of /dev/tty.
• test/integration/certmanager_test.go:511 — only the XDG leg is poisoned; the HOME leg resolves poisoned/.config/certmanager/config.yaml, which the test never creates, so a hermeticEnv that dropped the HOME= entry would still pass → write the poisoned config under poisoned/.config/certmanager/ too.
• nit: the --config '' rejection and the relative-XDG_CONFIG_HOME change are config-resolution work unrelated to the stdout gate → separate PR.

- `cmd/certmanager/main.go:53` — `ModeCharDevice` is set for `/dev/null`, so `certmanager > /dev/null` prints help and exits 0; a bare call with fd 1 closed does the same (the Go runtime reopens 0/1/2 onto `/dev/null`). Both contradict README's "prints help and exits zero only when stdout is a terminal" → detect with a tty ioctl (`golang.org/x/term.IsTerminal(int(f.Fd()))`), not a stat mode bit. - `cmd/certmanager/main_test.go:851` — `/dev/tty` open fails in a TTY-less CI container, so `TestIsTerminal` skips there, and every other test injects `interactive` through `runWith`; hardwiring `isTerminal` to `return false` leaves `go test ./...` entirely green. The true branch is unpinned in CI → open `/dev/ptmx` instead of `/dev/tty`. - `test/integration/certmanager_test.go:511` — only the XDG leg is poisoned; the HOME leg resolves `poisoned/.config/certmanager/config.yaml`, which the test never creates, so a `hermeticEnv` that dropped the `HOME=` entry would still pass → write the poisoned config under `poisoned/.config/certmanager/` too. - nit: the `--config ''` rejection and the relative-`XDG_CONFIG_HOME` change are config-resolution work unrelated to the stdout gate → separate PR.

unkin-agent added 3 commits 2026-09-22 23:29:38 +10:00

Detect a terminal with an ioctl, not a character-device stat ... 30eee88d95

/dev/null is a character device, and so is the /dev/null the Go runtime
reopens onto a closed fd 1, so a bare invocation printed help and exited
zero for both -- the two shapes a machine caller is most likely to hand
the binary.

Pin the invocation gate against every kind of stdout c6c3f6a092

Poison both config legs the hermetic env has to strip b0099991c0

unkin-agent commented 2026-09-22 23:43:19 +10:00

Author

Member

Copy Link

Copy Source

• cmd/certmanager/main_test.go:873 — run(nil, f, &stderr) leaves cobra to fall back to os.Args[1:], so this sub-case never exercises a bare invocation; under go test ./... -args foo it runs as certmanager foo and would issue against whatever /opt/certmanager/config.yaml resolves to on the host (verified: the exit-code assertions stop firing, only the isTerminal ones remain). → pass []string{}, as the other call sites and the PR body already claim.
• nit: test/integration/certmanager_test.go:292 — TestHermeticEnvSetsBothConfigLegs asserts only the shape of the returned env, so replacing hermeticEnv's body with os.Environ() still passes it on any host without ~/.config/certmanager/config.yaml. → assert the two values differ from the process's own HOME/XDG_CONFIG_HOME.
• nit: go.mod:7-8 — x/term v0.45.0 and x/sys v0.47.0 still declare go 1.25.0; only v0.46.0/v0.48.0 move to go 1.26.0. → the pin can move up one release without forcing the module's go` line.

- `cmd/certmanager/main_test.go:873` — `run(nil, f, &stderr)` leaves cobra to fall back to `os.Args[1:]`, so this sub-case never exercises a bare invocation; under `go test ./... -args foo` it runs as `certmanager foo` and would issue against whatever `/opt/certmanager/config.yaml` resolves to on the host (verified: the exit-code assertions stop firing, only the `isTerminal` ones remain). → pass `[]string{}`, as the other call sites and the PR body already claim. - nit: `test/integration/certmanager_test.go:292` — `TestHermeticEnvSetsBothConfigLegs` asserts only the shape of the returned env, so replacing `hermeticEnv`'s body with `os.Environ()` still passes it on any host without `~/.config/certmanager/config.yaml`. → assert the two values differ from the process's own `HOME`/`XDG_CONFIG_HOME`. - nit: `go.mod:7-8` — `x/term v0.45.0` and `x/sys v0.47.0` still declare `go 1.25.0`; only `v0.46.0`/`v0.48.0` move to `go 1.26.0`. → the pin can move up one release without forcing the module`'s `go` line.

unkin-agent added 4 commits 2026-09-22 23:49:08 +10:00

Pass an explicit empty argv so cobra cannot reach os.Args ... 21990d9297

A nil argv lets cobra fall back to os.Args[1:], which under
`go test -args foo` turns the bare-invocation cases into
`certmanager foo` and resolves /opt/certmanager/config.yaml. Assert
the bare-invocation error on stderr so the fallback cannot hide.

Puppet gives the child a temp file as fd 1, not a pipe; say so.

Assert the hermetic env replaces the ambient one, not extends it ... ddd93a8b71

The shape test passed on `return os.Environ()`; only a Vault-backed
test caught it. Move the helper and its test out from behind the
integration tag and reject any entry outside PATH/HOME/XDG_CONFIG_HOME.

Pin x/term v0.45.0 and x/sys v0.47.0 ... 173a82f29e

Newest pair still declaring go 1.25.0; v0.46.0/v0.48.0 declare
go 1.26.0 and break the golang:1.25 CI images.

Document the untagged hermetic-env helper 0860da03c4

unkin-agent added 1 commit 2026-09-22 23:49:35 +10:00

Rewrap the isTerminal comment d39ca1e3a6

benvin merged commit 2ba3e30bc3 into master 2026-09-24 21:14:38 +10:00

benvin deleted branch benvin/fix-invocation-guards 2026-09-24 21:14:39 +10:00

Sign in to join this conversation.

Reviewers

No Reviewers

Labels

No items

No Label

Milestone

No items

No Milestone

Projects

Clear projects

No project

Assignees

Clear assignees

benvin (BenVincent) unkin-agent (Unkin Agent) unkinben (Ben Vincent)

No Assignees

1 Participants

Notifications

Subscribe

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: unkin/certmanager#4