Repository navigation
Close ten engineering-standard conformance gaps - #31
Conversation
Mechanical gofmt sweep so the gofmt CI gate added later in this branch starts from a clean tree. No behaviour change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Closes GAP-1 and the structural half of GAP-5 from docs/engineering-profile.md. - LICENSE: MPL-2.0 verbatim. The README has claimed MPL-2.0 since the first commit with no licence file present, which made the claim unenforceable. - SECURITY.md: private reporting path, response targets, and an explicit scope that names the unauthenticated control plane (GAP-12) as a known limitation rather than letting people rediscover it as a finding (SEC-12). - CONTRIBUTING.md: setup, Conventional Commits, and the contract-change classification table from VER-3. - AGENTS.md: agent instructions per DOC-1. CLAUDE.md now points at it rather than duplicating, so the two cannot drift. - docs/adr/: MADR template, plus 0001 (the exit-code and output contract this branch implements) and 0002 (deferring the flowctl.v1 proto rename, with the CONF-1 exception recorded). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Implements ADR 0001. Closes GAP-3, GAP-9, GAP-15, GAP-17 and the foundation of GAP-2 and GAP-18 from docs/engineering-profile.md. New packages: - internal/exitcode: the CLI-15 code vocabulary, plus Of() which classifies gRPC statuses, context deadlines and filesystem errors so commands get a useful code without hand-classifying every call site. It walks the error chain, because status.FromError does not unwrap and commands wrap with %w. - internal/sanitize: strips C0/DEL/C1 from untrusted strings (CLI-22). The control plane accepts registrations from processes flowctl did not start, and printed their names and errors straight to the terminal; a crafted service name could emit OSC sequences. JSON output is deliberately not sanitised — encoding/json escapes control characters already, and stripping would corrupt data the consumer is entitled to receive intact. - internal/cliout: one Printer owning format selection, the JSON/NDJSON envelope with schema identifiers (CLI-6), stdout/stderr separation (CLI-5) and colour suppression (CLI-7). Behaviour changes: - Errors now exit 2-8 by cause instead of always 1. Unknown flags and unimplemented --output values exit 2; an unreachable control plane exits 7. - --output yaml|wide were advertised and never implemented; they are now a usage error rather than silently ignored. - FLOWCTL_ env prefix. viper.AutomaticEnv had no prefix, so it was binding bare OUTPUT, CONTEXT, NAMESPACE and LOG_LEVEL from the environment. - Config honours XDG_CONFIG_HOME, and layers a project flowctl.yaml over the user file. Config files are located by exact name: viper's SetConfigName search with a config type also matches extension-less files, and would try to parse the flowctl binary in the working directory as YAML. - Logs are JSON unless stdout is a colour-capable terminal, carry service and version, and timestamp in RFC 3339 UTC (OBS-1, DATA-3). - flowctl config show reports each setting with its source (CLI-9). - flowctl list drops --table/-t; table is the default and --output json replaces it. test/tools.go pins flowctl-sdk, which only test/testdata imports. The Go tool ignores testdata, so tidy dropped it and the delivery tests failed at run time. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Closes GAP-5, 6, 10, 11(partial), 16, 21(partial), 22 and advances GAP-2, 7, 18, 19 from docs/engineering-profile.md. CLI contracts - apiVersion is validated by one shared package (internal/apiversion). The CUE schema required flowctl/v1 but both Go loaders only checked non-empty, so 'apiVersion: v99' was rejected by validate and silently run as v1 by run — the reinterpretation VER-7 forbids. A test asserts the Go pattern and the CUE constraint describe the same language, so they cannot drift. - init --output is now --file/-f, and processors find --output is now --output-type (CLI-3). No deprecated aliases: a local --output shadows the global one, so an alias would make --output json mean 'write to a file named json' — the exact confusion the rule prevents. - The SIGINT handler is installed before startup rather than after it. A signal arriving during startup previously killed the process outright, orphaning child components and leaving the run row marked running. Bug fixes found by the new gates - Data race: GetComponentStatus and ListComponents returned pointers into the mutex-protected service map, so gRPC serialised a status struct while the health janitor mutated it. Both now clone. Found by go test -race on its first CI-equivalent run. - internal/core/pipeline.go discarded errors from src.Events and snk.Write behind '// TODO: Handle error'. A dropped sink write is data loss; both are now logged. - parseFileMode compared a byte against 0, which can never hold. The validation was still correct (the upper bound catches underflow) so this is dead code, not a vulnerability. Merge gate (§9) - gofmt, go vet, staticcheck, go test -race, govulncheck, gitleaks, buf lint, buf breaking against the last release tag, and a check that generated protobuf is current. - staticcheck.conf enables everything except ST1000, ST1003 and SA1019, each with a recorded reason and a gap ID. The other 32 pre-existing findings are fixed rather than excluded, so the gate is real. - A PR template prompts for the VER-3 change classification. Build and supply chain - toolchain pinned in go.mod (BUILD-2). - -trimpath everywhere, and the only embedded timestamp is the commit's. Deliberately not the ambient SOURCE_DATE_EPOCH: the Nix shell exports 315532800, which would stamp every build 1980. make verify-repro builds twice and compares hashes, and runs in the release gate. - goreleaser gains keyless cosign signing and SBOM generation (SEC-10); release.yml gains id-token: write and the release-gate checks. - Containers run non-root with OCI source/revision/version annotations. The duckdb-consumer Dockerfile also loses a builder stage that pulled a Go toolchain and discarded it. Also fixes the smoke scripts and docs that invoked the renamed flags, and rewrites CHANGELOG.md to cover the release history it previously omitted. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 206ac29b04
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| row := cliout.Row{ | ||
| "id": service.ServiceId, |
There was a problem hiding this comment.
When a registered component supplies an ID containing a C1 control such as U+009B, this value is passed unchanged to JSON/NDJSON output. Go's encoding/json escapes C0 characters such as ESC but emits U+0080–U+009F as raw UTF-8, so displaying flowctl list --output json in a terminal can still execute a crafted control sequence. Sanitize component-provided IDs and metric keys before placing them in output rows, including machine-readable formats.
AGENTS.md reference: AGENTS.md:L96-L99
Useful? React with 👍 / 👎.
| sigChan := make(chan os.Signal, 1) | ||
| signal.Notify(sigChan, syscall.SIGINT, syscall.SIGTERM) | ||
| defer signal.Stop(sigChan) |
There was a problem hiding this comment.
Registering sigChan here only buffers SIGINT/SIGTERM; nothing reads it or cancels r.ctx until startup has completely finished. If the signal arrives while components are launching or registering, the process continues starting work and can spend 30 seconds per component in waitForComponentRegistration—or indefinitely in another startup call—before honoring the shutdown request. Consume the signal concurrently during startup, cancel startup work, and run cleanup immediately.
Useful? React with 👍 / 👎.
| return err | ||
| } | ||
|
|
||
| format, err := cliout.ParseFormat(output) |
There was a problem hiding this comment.
When FLOWCTL_OUTPUT=json or a loaded flowctl.yaml sets output: json, Viper resolves that value, but the Cobra-bound output variable remains its table default unless --output was explicitly passed. Consequently config show reports the environment/file value while every command still renders a table. Parse viper.GetString("output") here (and likewise use the resolved value for other bound settings) after loading configuration.
Useful? React with 👍 / 👎.
The three gate jobs that failed on the first CI run each needed a different answer. govulncheck (SEC-9) — 13 reachable vulnerabilities, so the gate was working. Upgraded x/net to v0.60.0, grpc to v1.83.2, docker to v28.3.3 and mapstructure to v2.3.0, which clears 11 of them. The remaining two are Moby daemon issues with no fixed version published: an AuthZ plugin bypass and an off-by-one in plugin privilege validation. flowctl imports docker/docker only as a client (api/types/container, client, pkg/stdcopy), runs no daemon, serves no Docker API and manages no plugins; govulncheck reaches the module through package init alone. Rather than ignore the exit code, they are exempted individually in .vex/flowctl.openvex.json with a justification and an impact statement, and scripts/govulncheck.sh fails on anything not covered there — so the gate cannot quietly rot into a no-op. SEC-12 asks for exactly this. gitleaks (SEC-6) — the Action wrapper requires a paid licence for organization repos. The binary itself is open source, so CI installs a pinned release and runs it directly, scanning both the working tree and the commits a PR adds. buf lint (VER-5) — the STANDARD rule set flags pre-existing naming: RPC request/response types, a service suffix, shared Empty/Pong messages, and three *_UNKNOWN enum zero values. Every one requires renaming something that is visible on the wire or in generated Go, which is the migration already deferred in ADR 0002 — doing them piecemeal would spread one breaking change over several releases. Those five rules are excepted with reasons; everything else in STANDARD is enforced, and buf breaking is unaffected. Also makes buf the single protobuf generator. make generate and scripts/generate.sh previously shelled out to a local protoc, so the committed output depended on whichever plugin versions a developer had installed — which made the 'generated code is current' check unstable. Plugin versions are pinned in buf.gen.yaml; the 18-line diff under proto/ is the resulting version-header change, and regeneration is now idempotent. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Update: all seven gate jobs greenThe first CI run failed three jobs. Each needed a different answer, and two of them were the gate doing its job rather than the gate being wrong.
The last two are Moby daemon issues with no published fix — an AuthZ plugin bypass and an off-by-one in plugin privilege validation. flowctl imports
I also made buf the single protobuf generator.
|
The previous commit added internal/sanitize and wired it into cliout, but only three commands render through cliout. status, pipelines, chunks, processors and dashboard still printed component-supplied strings with raw fmt.Printf, so the CLI-22 hole was open on most of the surface — including chunks.go printing a component-relayed Error string verbatim, and dashboard writing component ids into a continuously redrawn TUI. Claiming GAP-9 closed was wrong; this actually closes it. Adds sanitize.Message, a protoreflect walk that returns a sanitised clone of any protobuf message: strings, repeated strings, map keys and values, recursing into nested messages, leaving bytes fields alone since they carry encoded payloads rather than display text. Every unconverted command now calls it on each control-plane response. Sanitising a clone rather than in place matters: a command that later gains JSON output must emit the original bytes, because encoding/json escapes control characters already and stripping them would corrupt data the consumer is entitled to receive intact. The comment at each call site says to drop it when the command moves to cliout, which sanitises at the point it formats a cell — the better boundary. Two approaches were rejected. Wrapping ~70 print sites individually is easy to get wrong and leaves future prints uncovered. Wrapping stdout in a filtering writer would also strip flowctl's own intentional ANSI. Verified end-to-end rather than only by unit test: registering a service named "evil\x1b]0;PWNED\x07\x1b[2Kbenign" with metadata keys and values carrying an OSC 52 clipboard write, then byte-scanning stdout of list, status, processors list, pipelines list and chunks list. Zero ESC bytes on every path; the name renders as inert printable text. That scan also caught a CLI-7 violation the earlier commit missed: status emitted raw ANSI colour and a screen-clear unconditionally, so redirecting it to a file embedded escape codes and piping it to grep could fail to match. Colour there now resolves once through cliout.ShouldColor. dashboard is left to lipgloss's own detection. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Correction: the escape-injection fix was incompleteAsked to confirm the five bugs were resolved, I re-checked each against the committed code rather than from memory. Four were fully fixed. The escape-injection one was not, and I had described it as though it were.
Now actually fixed, in
Cloning rather than mutating matters: a command that later gains JSON output must emit the original bytes, because Two alternatives rejected: wrapping ~70 print sites individually is easy to get wrong and leaves future prints uncovered; wrapping stdout in a filtering writer would also strip flowctl's own intentional ANSI. Verified end-to-end, not just by unit test. Registered a service named The name renders as inert printable text: That scan also caught a CLI-7 violation the earlier commit missed. All seven gate jobs green. |
…e to cliout Advances GAP-2 from 3 of 14 commands to 11. Each converted command now emits --output json and --output ndjson with a schema identifier, and drops its sanitize.Message call: cliout strips control characters as it formats a cell, and machine output must keep the original bytes. New schemas, all contracts under VER-3: flowctl.processor_list.v1 / flowctl.processor.v1 flowctl.chunk_run_list.v1 / flowctl.chunk_run.v1 flowctl.pipeline_list.v1 / flowctl.pipeline.v1 flowctl.pipeline_run_list.v1 / flowctl.pipeline_run.v1 flowctl.pipeline_status.v1 flowctl.translation.v1 Machine output carries full values where the table condenses: every event type rather than "first (+2)", raw counts, real timestamps, and health as a stable lowercase string. The table layout is explicitly not a contract. BREAKING: translate --output/-o is now --target/-t. It selected a deployment target, not an encoding, so it shadowed the global --output — the third instance of the CLI-3 collision, after init and processors find. Docs and examples updated. status keeps its grouped human layout, which no generic renderer would reproduce, and emits a single document in machine mode carrying the summary and every component — so a caller needs one call, not two. Its --watch is now a usage error when combined with a machine format, since redrawing a terminal has no meaning for a consumer reading one document. Two latent bugs fixed on the way: - pipelines sliced run.RunId[:8] for its short-id column, which panics on any id shorter than eight bytes. The control plane is a trust boundary and the id comes off the wire; shortID now slices by rune count and tolerates short input. - status built its component groups twice, once in a loop whose appends were then discarded. Caught by staticcheck. Verified against a live control plane: every converted command emits a parseable envelope, the grouped status table still renders, exit codes hold (3 for an unknown processor, 2 for --watch with json), and a service registered under a hostile name still produces zero ESC bytes on stdout across all five list paths. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Closes GAP-2 and GAP-18. Every command that produces records now emits --output json and --output ndjson with a schema identifier; the twelve schemas are listed in CHANGELOG.md and are contracts under VER-3. Converted in this commit: validate, init, sandbox status. Guarded or routed: dashboard, run, server, the --show-status live monitor, and the component downloaders in internal/registry and internal/components. Commands that produce no records say so rather than pretending: - dashboard is a full-screen TUI. It rejects a machine format and points at 'flowctl status --output json', and refuses to run at all when stdout is not a terminal instead of writing escape codes into a pipe. - run and server are long-running. Their startup banners were on stdout and are now on stderr, so 'flowctl run' writes nothing to stdout. BREAKING: translate --output/-o is now --target/-t. That was the third instance of the CLI-3 collision, after init and processors find. Also fixed while converting: - init prompted without checking whether stdin is a terminal, so piping anything into it read EOF for every answer and silently built a pipeline from defaults. It now errors with a pointer to --preset and --non-interactive (CLI-8). In table mode it prints just the created path, which makes `flowctl run "$(flowctl init --preset ...)"` work. - validate exited via a bare os.Exit(1), bypassing the exit-code contract; it now exits 8 on failure and 3 on a missing file. It also emits a result document even when the file fails to load, so a caller never has to handle two shapes. - translate reported an incomplete pipeline as a generic failure; a missing image is a data validation failure, so it exits 8. - The live monitor and status cleared the screen unconditionally. Writing escape codes into a redirected stream corrupts it (CLI-7). After this there is no bare fmt.Print* to stdout left anywhere outside one TTY-gated screen clear. Verified against a live control plane: all ten record-producing commands emit exactly one parseable document under --output json, zero ESC bytes reach stdout on any path, and exit codes hold (2 for a machine format on dashboard or init, 3 for a missing file, 8 for failed validation). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…erator test to the contract Two problems the race-detector run surfaced. grpc.NewClient replaced grpc.DialContext+WithBlock during the conversion, and NewClient connects lazily: a dead control plane no longer failed at dial time, so formatPipelinesControlPlaneError never ran and the most common error an operator hits degraded to a bare "connection refused". cliout.UnreachableInterceptor restores the actionable message on every unary call, in one place rather than at each of the seven call sites, and keeps the Unavailable status so the exit code is still 7. TestFlagshipOperatorFlow asserted on the exact column spacing of pipelines run-info — "Pipeline: operator-smoke" — which ADR 0001 explicitly declares is not a contract. Now that --output json exists the test binds to the schema instead, and checks the human path only for content rather than layout. The new runCommandJSON helper captures stdout and stderr separately and parses stdout alone, so the test asserts CLI-5 as a side effect: any progress line leaking onto stdout now fails the parse. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
GAP-2 and GAP-18 closed: the output conversion is finishedEvery command that produces records now emits
Machine output carries full values where the table condenses: every event type rather than Commands that produce no records say so rather than pretending. There is no bare Third
|
* fix(security): require Go 1.26.10 for the crypto/x509 fix GO-2026-6809: compromised intermediate certificates were not distrusted by crypto/x509. Fixed in go1.26.10; we required 1.26.9. flowctl reaches it through TLS and mTLS to the control plane — govulncheck traces it via x509.Certificate.Verify — so this is reachable rather than merely linked. Nothing in this repo changed to cause it. The advisory was published after #31 ran, and the vulncheck gate added in that PR caught it on the first run against main. That is the gate working as intended: it pulls a live vulnerability database, so it can go red without a code change. Raises the go directive rather than only the toolchain line, so an older toolchain refuses to build: go: go.mod requires go >= 1.26.10 (running go 1.26.9; GOTOOLCHAIN=local) With only a toolchain line and a lower go directive, GOTOOLCHAIN=local silently builds against the affected standard library, which is the outcome worth preventing for a security fix. No separate toolchain line is kept: it is redundant once the two match and 'go mod tidy' — which CI enforces — removes it. Note for Nix users: nixpkgs ships Go 1.26.6-1.26.9, so the dev shell's Go is older than the minimum and Go downloads 1.26.10 itself. Recorded against GAP-21 in the engineering profile, which already tracks CI and the flake disagreeing about the toolchain. Verified: govulncheck clean, lint clean, full suite and -race green across all 19 packages, and the build still reproduces byte-for-byte. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * build(nix): move the dev shell to Go 1.27.3 The dev shell was on go_1_26, which nixpkgs ships as 1.26.9 — older than the 1.26.10 this branch now requires. Every Nix developer would have relied on Go downloading a toolchain itself. No flake.lock change: the pinned nixpkgs already carries go_1_27 as 1.27.3, a stable patch release. An earlier note on this PR claimed it was 1.27rc3; that came from the local flake registry, which is stale, not from the lock. staticcheck had to be rebuilt with the same Go. It links go/types, so a binary built with 1.26 cannot parse the 1.27 standard library, and make lint failed in the shell with 'method must have no type parameters' from math/rand/v2. Not a version problem — nixpkgs has the current 2026.2.2 — so the fix is the override rather than a bump. CI was never affected; it uses dominikh/staticcheck-action independently of Nix. Verified inside nix develop: Go 1.27.3, lint clean, 19 packages green, govulncheck clean, build still reproduces byte-for-byte. Records GAP-26 for a pre-existing failure found on the way: nix build .#default runs protobuf generation without the protoc plugins in nativeBuildInputs and fails identically on main. nix develop, the path BUILD-1 documents, is unaffected. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
What and why
Brings flowctl into conformance with the OBSRVR Engineering Standard v2.0 on as many points as can be done in one change, and publishes
docs/engineering-profile.mdso the remaining gaps are tracked rather than rediscovered.flowctl is classified in §13 as network service + CLI tool, so §2–12 all apply. The audit that started this put the repo at roughly 25–30% conformant. This closes ten gaps outright and advances six more.
Four commits, meant to be reviewed in order. The first is a pure
gofmtsweep so it doesn't pollute the rest.Gaps closed
LICENSE(MPL-2.0 — the README has claimed it since the first commit with no file present),SECURITY.md,CONTRIBUTING.md,AGENTS.mddocs/adr/with two ADRs, PR template for change classificationapiVersionvalidator instead of three that disagreed-race, govulncheck, gitleaks, bufFLOWCTL_env prefix, XDG config,config show--outputno longer means three different thingsNO_COLOR,TERM=dumb, TTY detectionAdvanced but not closed: GAP-2 (3 of 14 commands converted to the new output path), GAP-7 (buf in CI; the
flowctl.v1rename deferred under ADR 0002), GAP-11, GAP-18, GAP-19, GAP-21.Three real bugs the new gates found
These are the argument for the gates, so they're worth calling out separately:
GetComponentStatusandListComponentsreturned pointers into the mutex-protected service map, so gRPC serialised a status struct while the health janitor was writing to it.go test -racecaught it on its first run. Both now clone.Plus two smaller ones:
internal/core/pipeline.godiscarded errors fromsrc.Eventsandsnk.Writebehind a// TODO(a dropped sink write is data loss), andparseFileModehad abyte < 0comparison that could never be true.Contract classification (VER-3)
flowctl is at v0.0.12; VER-2 permits a minor bump to break before 1.0. Every break is in
CHANGELOG.md:$? -eq 1should test$? -ne 0.flowctl init --output <file>→--file/-f.flowctl processors find --output <type>→--output-type.flowctl list --table/-tremoved; table is the default.--output yamland--output widenow error instead of being silently ignored.apiVersion: v1is rejected; useflowctl/v1. It was already rejected byflowctl validate— onlyflowctl runaccepted it, and ran it as v1 anyway.Neither flag rename keeps a deprecated alias, which is a deliberate call: a local
--outputshadows the global one, so an alias would make--output jsonsilently mean "write to a file namedjson" — the exact confusion CLI-3 exists to prevent.ADRs: 0001 (output and exit codes), 0002 (deferring
flowctl.v1, with the CONF-1 exception recorded).On the linter exclusions
staticcheck.confexcludes exactly three checks —ST1000,ST1003,SA1019— each with a written reason and a gap ID. The other 32 pre-existing findings are fixed, not excluded, so the gate is real rather than decorative.SA1019is thegrpc.DialContext→NewClientmigration, which isn't mechanical (NewClientnever blocks) and needs its own change.Testing
make lintpasses (gofmt, go vet, staticcheck — zero findings)make testpassesmake test-racepasses across every packagemake verify-repro— two builds, identical hashesNew tests: golden tests for every output envelope, exit-code mapping including wrapped gRPC statuses, terminal-injection cases drawn from real OSC sequences, and fuzz tests for the sanitiser and
apiVersion(the latter found a zero-padding case that corrected the test's invariant).Not in this PR
flowctl.v1rename — both wire-breaking, so they should cost one coordinated migration, not two.docs/engineering-profile.mdhas the full register with status on all 25 gaps.🤖 Generated with Claude Code
https://claude.ai/code/session_01H3BjiVyh7ifuS7xSrRUe2m