chore: typography cleanup + polish#46
Merged
Merged
Conversation
aa295df to
d9e3c60
Compare
7460e41 to
642f014
Compare
Two concerns rolled into one PR because they share the same files and
the polish changes were caught while reviewing the typography sweep.
## Typography cleanup
Em-dashes (U+2014) are an LLM tell and clash with the project's
plain-ASCII voice. Rephrase case-by-case across all tracked files:
colon, parens, semicolon, or sentence split per what reads better.
En-dashes inside numeric ranges (10-19, 20-29, 1-9) are kept; they are
typographically correct and out of scope.
The 'PR 1' parenthetical in cmd/kfeatures/main.go (a leftover stack
position reference) is dropped: internal stack-position refs do not
belong in landed code.
User-visible format change in lockstep:
- cmd/kfeatures/main.go's 'kfeatures check' FeatureError path now
prints 'FAIL: feature - reason' (was 'FAIL: feature -- reason' with
em-dash). The contract is mirrored in CHANGELOG.md '[Unreleased]'
and CONTRIBUTING.md 'Two error contracts' so docs stay accurate.
- README.md usage examples that print the same kind of message switch
to ASCII hyphen.
## Post-self-review polish
Self-review of the cli-track stack surfaced four small defects that
fit naturally alongside the typography pass:
- test/cli_mcp.bats: mcp_call no longer redirects stderr to /dev/null.
It captures stderr and surfaces it on non-zero exit so future
panics, structcli envelope errors emitted before the MCP loop, or
in-MCP diagnostics are visible at test-failure time instead of
manifesting as a confusing 'no response with id N' assertion.
- test/cli_common.bats: 'check: legacy alias is rejected' asserted
on a JSON-escaped substring of structcli's prose 'message' field.
Switched to assert on the structured 'got' / 'flag' / 'error'
fields, which are the documented contract.
- cmd/kfeatures/main.go inMCPMode: comment now spells out the
coupling to structcli's MCP wrapper being the sole SetOut caller
(true at v0.17.0; latent risk if that changes) and the
no-defer-in-os.Exit-RunE invariant (true today; comment exists to
keep it true).
- cmd/kfeatures/main.go mcpServerVersion: comment previously claimed
it 'mirrors what kfeatures version prints', which is wrong (the
CLI version subcommand emits a richer 'kfeatures <ver> (<commit>)
built <date>' line; MCP serverInfo carries just the version
string). Comment now says what it actually does and why the two
paths differ.
## Tested
- go build / go vet / go test -count=1 ./... green
- bats test/cli_common.bats test/cli_linux.bats test/cli_mcp.bats:
34/34 green, including the renamed 'mcp: stdout is JSON-RPC only,
no leakage from command handlers' test
- mcp_call stderr-surfacing verified by feeding malformed input to
/tmp/kfeatures --mcp: now reports 'kfeatures --mcp exited 1;
stderr: {error envelope}' instead of swallowing it
Co-authored-by: Ona <no-reply@ona.com>
d9e3c60 to
01cea4c
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on #45. Top of the cli-track stack now (#38 → #39 → #40 → #41 → #42 → #43 → #45 → this). The base will retarget downward as each parent merges.
Two concerns rolled into one PR because they share the same files and the polish changes were caught while reviewing the typography sweep.
Part 1: typography cleanup
Em-dashes (
—, U+2014) used liberally across docs and a couple of source files. They are an LLM tell, and they don't match the project's plain-ASCII voice (the rest of the repo uses colons, parens, and sentence splits). The user-visible CLI outputFAIL: feature — reasonfromkfeatures checkalso carried one.Plus: one leftover
(PR 1)parenthetical in a comment incmd/kfeatures/main.gothat referred to an internal stack position. References to internal-only files (spec.md,spec-bpffs-mount.md,where-the-real-value-of-structcli-v017-is.md) were already absent from tracked content; verified by scan.Scope
All tracked files were scanned, not just
*.md:En-dashes inside numeric ranges (
10–19,20–29,1–9in the exit-code documentation) are kept. They are typographically correct (Keep-a-Changelog style) and out of scope per F1's literal definition (em-dash, U+2014, only).Approach
Case-by-case:
term — definitionlist items →term: definition(colon).User-visible format change (in lockstep)
cmd/kfeatures/main.gopreviously printedFAIL: %s — %s\nonFeatureError. It now printsFAIL: %s - %s\n. The contract is documented in two places that change together:CHANGELOG.md[Unreleased](line that documents the--mcp/WithFlagErrorsstory for thekfeatures checkbusiness-outcome contract).CONTRIBUTING.md## CLI conventions→ "Two error contracts" subsection.The bats suite (
cli_common,cli_linux,cli_mcp) does not assert on the em-dash glyph, so behaviour stays green.The one renamed test (
mcp: stdout is JSON-RPC only — no leakage from command handlers→mcp: stdout is JSON-RPC only, no leakage from command handlers) passes unchanged.Part 2: polish
After the typography sweep landed, a self-review of the cli-track stack surfaced four small defects that fit naturally alongside the typography pass (same files, same review batch, no extra reviewer churn).
Issue:
mcp_call 2>/dev/nullmasked failures (test/cli_mcp.bats)The MCP test harness redirected stderr to
/dev/null, so any panic, structcli envelope error emitted before the MCP loop, or in-MCP diagnostic was invisible at test-failure time. A regression would manifest asno response with id Nfrom the response-extractor instead of the actual error.Demonstrated by feeding malformed input to
/tmp/kfeatures --mcp:…which the old harness silently dropped.
Fix:
mcp_callnow captures stderr to a temp file and surfaces it on non-zero exit:Issue: fragile JSON-escape assertion (
test/cli_common.bats)The
check: legacy alias is rejectedtest asserted on a JSON-escaped substring of structcli's prosemessagefield:assert_output --partial 'unknown feature: \"bpffs\"'That ties the test to structcli's specific escaping convention. Any future structcli rewording (or change to escape policy) breaks it even when behaviour is correct.
Fix: assert on the structured
error/flag/gotfields, which are the documented contract ofStructuredError:Issue:
inMCPModecoupling and defer-safety undocumented (cmd/kfeatures/main.go)inMCPMode(c)returnstrueiffc.OutOrStdout() != os.Stdout. This works because the only code path that callscmd.SetOutin structcli v0.17.0 is the MCP wrapper (mcp.golines 399 and 417). Verified by grepping the structcli source for non-test callers: zero hits outsidemcp.go.The proxy is latent-fragile: any future structcli capability that calls
SetOutfor another reason would silently flipinMCPModetotrueoutside MCP, causing thecheck/configos.Exit(1)carve-out to be skipped and the typed errors to be printed as structured-error envelopes (collapsing the documentedFAIL: feature - reasonand{ok,feature,reason}contracts).Separately:
os.Exit(1)inside aRunEskips deferred functions. There are nodefers in thoseRunEs today; adding any in a future PR (file close, lock release, telemetry flush) would silently leak.Fix: comment block on
inMCPModenow spells out:defer-in-os.Exit-RunEinvariant and why the comment exists.No code change. The comment is the deliverable: it preserves the invariants for future readers (human or agent).
Issue: misleading
mcpServerVersioncomment (cmd/kfeatures/main.go)The doc comment claimed
mcpServerVersion()"mirrors whatkfeatures versionprints". That is wrong:kfeatures versionprintskfeatures <ver> (<commit>) built <date>(orkfeatures (dev)as fallback).mcpServerVersion()returns just the bare version string (or"dev"), which goes into MCP'sserverInfo.versionfield; thenamefield already carries"kfeatures".Both are correct for their respective protocols (MCP separates name/version; the CLI is a single human-readable line). The comment was the wrong part.
Fix: comment now accurately describes the function's purpose and notes why MCP and CLI render differently.
Diff shape
Tested
go build ./...✓go vet ./...✓go test -count=1 ./...✓ (forced fresh, both packages green)bats test/cli_common.bats test/cli_linux.bats test/cli_mcp.bats✓ (34/34, including the renamed mcp test)mcp_callstderr-surfacing demonstrated against malformed-input invocationSelf-review notes (intentionally NOT addressed in this PR)
The self-review also surfaced items that I deliberately did not act on, so reviewers know they were considered:
bash/zsh/fish/powershell). A fifth would be a cobra-version-bump event surfaced immediately ontools/list. Theoretical, not a defect.ProbeWithcaching interaction with MCP server mode. Initially flagged. Re-verification:ProbeWithdoes not cache (onlyProbe()does), so each MCPtools/callgets a fresh probe. Non-issue.ValidationFailed,InvalidFlagEnum,Config*,Env*) are not reachable from kfeatures' surface (no struct validators, no config files, no env-only flags). Table is accurate for kfeatures.Out of scope (won't be in this PR even as a follow-up)
spec.md,spec-bpffs-mount.md,where-the-real-value-of-structcli-v017-is.mdare working notes on disk but not tracked in git. They remain local-only.U+2014unicode characters (en-dashes in numeric ranges, the existing⇒arrows inAGENTS.mddoc-split section, the→in PR-stack notation in PR bodies) are intentional and untouched.