Skip to content

docs+quality: CLI reference drift, one ErrHandled, real flags for the hand-parsed leaves, parity holes (2026-09-05 review) - #172

Merged
fentas merged 2 commits into
feat/go-single-binaryfrom
fix/review-docs-quality
Sep 6, 2026
Merged

docs+quality: CLI reference drift, one ErrHandled, real flags for the hand-parsed leaves, parity holes (2026-09-05 review)#172
fentas merged 2 commits into
feat/go-single-binaryfrom
fix/review-docs-quality

Conversation

@fentas

@fentas fentas commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

Docs and small-quality findings of the 2026-09-05 lok8s review (code-quality / docs / feature-completeness slice). Stacked on #171 (fix/review-supply-chain) because both touch internal/cli/cmd_kubehz.go; retarget to feat/go-single-binary once #171 merges.

Docs

  • docs/reference/cli.md: lo up --ci [--timeout] with the tilt ci exit-status contract; lo tilt ci|preflight (flags verified against --help); lo kubehz join <node> [--print-token]; lo kubehz claim --nonce - / KUBEHZ_CLAIM_NONCE; the argsh#176 footnote dropped (the binary reads stdin itself); lo kustomize test names its prerequisite; the lo up "legacy fallback" sentence replaced by what the binary does (verified in internal/provision/spec.go: ResolveSpec stops with No cluster.lok8s.yaml or deploy.lok8s.yaml found; no spec-less path exists).
  • README: lo status → "Cluster health (the driver's status check)"; lo up --ci, lo addons [name...] [--detail], lo tilt …|ci|preflight; the .mcp.json sentence says it launches the argsh-builtin server.
  • docs/guide/toolchain.md: "each step is an owner decision" → "planned; sequenced separately".
  • mise.toml: PATH_SECRETS export commented out with the reason.
  • /mnt/scratch/github/kernpilot/TODO.md: the lo audit --sarif line struck (shipped in feat(audit): lo audit --sarif — SARIF 2.1.0 output for GitHub code scanning #163) — edited directly, it is not in a repo.
  • docs/reference/go-migration.md: D23 — a -s placed before the subcommand binds to the leaf's own -s on secrets set|env and handover receive (cobra is position-insensitive; argsh's main consumed it as --cluster). (A first push recorded a "B6" for lo image clean; the reviewer showed it was the documented D3 flag-placement effect — withdrawn, see "Review round".)

Quality

  • Short texts of the ported addons / kustomize builders come from the spec; TestCommandTreeMatchesArgshUsage now compares the assembled tree's Short too (the gate's boundary is stated in the test: flags/arity/long help stay outside).
  • One ui.ErrHandled: all 15 package sentinels alias it; the six == compares are errors.Is. Mechanical, behaviour unchanged.
  • argshErrorf moved to root.go next to ErrHandled.
  • Real cobra flags for lo secrets set|env, lo kubehz handover receive, lo env services--help, completion and the ophis MCP schema see them now. The bash first-match shorthand semantics are kept: -s = --namespace / --snapshot on those leaves; a local cluster flag (same name, no shorthand) makes cobra skip the inherited global instead of panicking on the redefinition. env services keeps the opposite resolution (inherited -s/-r win) exactly as the old comment documented. Error text goes through argshFlagErrors. The parser unit tests became flag-binding tests through the assembled tree.
  • lo kustomize <unknown> now prints Invalid command: x and exits 1 (cobra printed the group help and exited 0); same message as bash (rc 2 there, D1).
  • Parity holes closed: lo trust (stubbed mkcert in a dedicated project; the real one would install a CA into the ambient trust store), lo kustomize list|clean|bogus, lo image clean (--domain before the verb, D3), lo tilt restart, lo kubehz claim-code.
  • Shorthand flag errors now carry the argsh wording tree-wide: argshFlagErrors rewrites pflag's unknown shorthand flag: 'x' in -xunknown flag: -x and flag needs an argument: 'n' in -nmissing value for flag: <long name>; long forms already matched. lo mcp is Go-only (allowlisted in the drift gate) so it has no bash twin to diff; lo kustomize build|test need a Go toolchain in the synthetic project and stay out.
  • internal/execx unit tests: Look order (.bin over PATH, non-executables skipped, miss reported), Run env merge, Dir, path-as-is, unresolvable tool.

Gates (run locally with env -u PATH_*, on the rebased head)

gate result
make build / make build-full ok
go vet ./... (core + inprocess) ok
go test ./... -count=1 (core + inprocess) ok
golangci-lint run (core + --build-tags inprocess) 0 issues
ten hack/parity-*.sh × bin/lo and bin/lo-full 20/20 "parity: all checks passed"
./.bin/argsh test tests/unit/ · tests/operator/ 1..1448 ok · 1..37 ok
argsh-lint on the three edited harnesses rc 0 each (hack/lint-shell.sh as a whole: rc 123 on the pristine base too — pre-existing AG007 warnings in untouched files)

Mutation checks

Hard-coded addons Short → TestCommandTreeMatchesArgshUsage FAIL; -s shorthand removed from secrets setTestSecretsSetEnvFlagsBindLikeTheBashSpec FAIL; -s removed from handover receiveTestHandoverReceiveFlagsBindLikeTheBashSpec FAIL; Look no longer prefers .binTestLookPrefersTheProjectBinOverPATH FAIL; Run drops the inherited env → TestRunMergesEnvAndSetsDirAndUsesPathsAsIs FAIL; the kustomize RunE was proven by the harness going red before it existed (lo kustomize bogus — rc=0, want 1).

Review round

Reviewer verdict on the first push: REQUEST CHANGES — the B6 row misdiagnosed the image clean difference (it is D3: bash resolves the spec from the domain parsed before dispatch, so a trailing --domain is invisible there). Fixed: --domain first in the harness, no allow-list, B6 withdrawn. Should-fixes taken: stale cmd_env.go header, shorthand error wording normalised in argshFlagErrors (tests pin the bash text, mutation-checked), D23 documented. Nits taken: secretsArgs comment, test moved to cmd_secrets_test.go, cli.md --print-token wording, drift-test comment says "top-level only". All gates re-run on the fixed head (table above reflects it).

Skipped (from the brief, on purpose)

The env/exec seam migration, the three spec.bootstrap parsers, lo secrets remove-key (#137 needs the bash twin first), the long-function splits. TODO(go-port)TODO(WP3)/(WP5): no TODO(go-port) exists on this head any more (grep is empty), nothing to rename.

🤖 Generated with Claude Code

@fentas
fentas changed the base branch from fix/review-supply-chain to feat/go-single-binary September 6, 2026 06:33
fentas and others added 2 commits September 6, 2026 08:59
… hand-parsed leaves, parity holes

Docs (2026-09-05 review, quality/docs slice)
- docs/reference/cli.md: `lo up --ci/--timeout` (+ the tilt ci exit-status
  contract), `lo tilt ci|preflight`, `lo kubehz join <node> [--print-token]`,
  `lo kubehz claim --nonce -` / KUBEHZ_CLAIM_NONCE, the argsh#176 footnote
  gone (the binary reads stdin itself), `lo kustomize test` names its
  prerequisite (make + Go), and the `lo up` paragraph states what the
  binary does without a spec (stops on ResolveSpec; no legacy fallback).
- README: `lo status` no longer promises per-target build state; `lo up
  --ci`, `lo addons [name...] [--detail]`, `lo tilt ci|preflight`; the
  `.mcp.json` clause says it launches the argsh-builtin server.
- docs/guide/toolchain.md: "owner decision" → "planned".
- mise.toml: the deprecated PATH_SECRETS export is a comment now.
- docs/reference/go-migration.md: B6 — `lo image clean` names the cache
  registry from different sources in the two implementations (found by
  the new harness line; not aligned here).

Quality
- Ported `addons` / `kustomize` builders take their Short from the spec;
  TestCommandTreeMatchesArgshUsage now also compares the ASSEMBLED tree's
  Short texts (mutation-checked: a hard-coded Short goes red).
- One ui.ErrHandled: every package sentinel (cli, tilt, scaffold, kubehz,
  recover, operator, env, image, hooks, build, deploy, addons,
  secrets/lint/oidc ErrPrinted) is the same value; the six `==` compares
  are errors.Is. Behaviour unchanged (the mapping shims stay).
- argshErrorf lives next to ErrHandled in root.go.
- Real cobra flags for `lo secrets set|env`, `lo kubehz handover receive`,
  `lo env services` (help, completion and the MCP schema see them). The
  bash shorthand shadowing (-s = --namespace / --snapshot) is kept by
  declaring a local `cluster` flag so cobra skips the inherited one; the
  argsh error text comes through argshFlagErrors. Flag-binding tests
  replace the parser tests (mutation-checked).
- `lo kustomize <unknown>`: `Invalid command: x`, rc 1 (was: group help,
  rc 0) — the `ai` group's shape.
- hack/parity-*.sh gain: trust (stubbed mkcert), kustomize list/clean/
  bogus, image clean, tilt restart, kubehz claim-code.
- internal/execx has unit tests: Look prefers .bin over PATH and skips
  non-executables, Run appends Env, honours Dir, uses a path as-is, and
  reports an unresolvable tool (mutation-checked).

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…for shorthand flag errors

- The `lo image clean` "divergence" recorded as B6 was the documented D3
  flag-placement effect: bash main resolves the spec (and with it the
  registry network name) from the domain it parsed BEFORE dispatch, so a
  --domain after the verb is invisible to that lookup in bash only. The
  harness line now puts --domain first, needs no allow-list, and B6 is
  gone from the catalogue.
- argshFlagErrors rewrites pflag's SHORTHAND messages into argsh's
  (`unknown flag: -x`, `missing value for flag: <long name>`); the long
  forms already matched. The flag-binding tests pin the bash wording
  (mutation-checked: normalisation off → FAIL).
- D23: a `-s` placed before the subcommand binds to the leaf's own `-s`
  on secrets set/env and handover receive (cobra is position-insensitive;
  argsh's main consumed it as --cluster).
- cmd_env.go header no longer describes the deleted hand-parser;
  secretsArgs' comment names what it is; the secrets flag test lives in
  cmd_secrets_test.go; the drift-test comment says "top-level only";
  cli.md's --print-token line matches the flag text.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@fentas
fentas force-pushed the fix/review-docs-quality branch from 38f03e3 to 3a106bc Compare September 6, 2026 06:59
@fentas
fentas merged commit c91d0ed into feat/go-single-binary Sep 6, 2026
17 checks passed
@fentas
fentas deleted the fix/review-docs-quality branch September 6, 2026 07:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant