fix(telemetry): stop shipping raw CLI args in ao.cli.usage_errors (#2813)

[Pritom14]

What

Fixes #2813. ao.cli.usage_errors recorded command/command_path as the raw positional args of the failed invocation. Because parsing already failed, whatever the user typed went verbatim to PostHog — URLs, absolute file paths, multi-paragraph instructions, non-English text — and the remote allowlist gates only keys, not values.

Fix (primary + defense-in-depth)

Client — cli.usageErrorCommand (root.go): only propagate tokens that are registered command names (collected from the Cobra tree). The first non-flag token that isn't a known command becomes the sentinel <unknown> and the scan stops, so raw positional text never leaves the machine. A known command followed by a bad arg keeps the command name and records <unknown> for the arg (e.g. ao list some-project → command=list, command_path=ao list <unknown>).

Sink — telemetry.sanitizeRemoteValue (posthog.go): command-shaped keys (command/command_path) whose value isn't command-shaped are replaced with a stable, non-reversible sha256:<16> token; all allowlisted strings get a length backstop. Catches any future emitter that reintroduces the leak.

Tests

• usageErrorCommand: URLs, absolute paths, prose, non-ASCII → <unknown>; known-command-then-free-text keeps the command and redacts the arg; asserts no raw token ever appears in output.
• sanitizeRemoteValue: non-command-shaped command values → sha256: token (raw text never present); real commands + the <unknown> sentinel pass through; length backstop applied.
• go test ./internal/cli/ ./internal/adapters/telemetry/, go vet, gofmt all clean.

🤖 Generated with Claude Code

[i-trytoohard]

Review — PR #2816 (fixes #2813)

Technical review from the issue author. Per repo policy I'm posting as comment, not approval — final sign-off should come from an authorized maintainer (@yyovil / @dhruv / @Priyanchew et al.).

Could not run go build/go vet/go test myself — no Go toolchain on this box. Findings below are from reading the diff + call-graph tracing. All 9 CI checks pass (build-test, lint, format, vet, api-drift, scan, native mac/linux/win, container), so the author's "go test/vet/gofmt clean" claim holds.

✅ Verified correct (traced end-to-end)

• Client fix resolves the reported leak. ao "https://gitlab.com/…/-/merge_requests/9" → the URL isn't in knownCommandNames → usageErrorCommand emits command="<unknown>", command_path="ao <unknown>" and stops scanning. Raw URL never propagated. Same for absolute paths, prose, non-ASCII — all covered by TestUsageErrorCommandRedactsFreeText with a hard "no raw arg in output" assertion.
• Known-then-bad-arg case is handled and actually better than before. ao status extra → old code returned command="extra" (last raw token); new code returns command="status", command_path="ao status <unknown>". Safer and more accurate telemetry.
• Sink defense works as advertised. commandShapedKeys covers all three events that carry command/command_path (ao.app.active, ao.cli.invoked, ao.cli.usage_errors) — no false-positive risk on unrelated events. A non-command-shaped value in those keys → sha256:<16> token; raw text absent (TestSanitizeRemoteValueRedactsNonCommandShapedCommandKeys).
• The error field is not a leak. The daemon route never includes error in the emitted payload (not in the allowlist; router.go:212-219 emits literal error_kind: "usage" instead). The CLI still POSTs err.Error() over loopback, but it stays local. Existing telemetry_test.go:92-94 already guards this.
• Scope hygiene: 4 files, single commit, no drive-by changes, no go.mod/deps churn. Package doc + inline comments reference #2813.

⚠️ Non-blocking observations (none merge-blocking)

1. No end-to-end test through the daemon route for the sink defense. TestSanitizeRemoteValue* tests the sanitizer directly, and TestCLIUsageErrorRouteEmitsTelemetry only POSTs a clean "status". There's no test POSTing a URL as command to /internal/telemetry/cli-usage-error and asserting the captured sink event shows sha256:…. That would close the loop on the defense-in-depth path (catches a future regression where the route wires the value around the sanitizer). Medium.
2. isCommandShaped carves out </> for the <unknown> sentinel rather than special-casing the literal string. Slightly hacky — a value like "a<b>" would pass as command-shaped. Practically harmless (bounded to 48 chars, no //:/non-ASCII), but special-casing "<unknown>" would be cleaner. Minor/style.
3. Two length caps are partially redundant. isCommandShaped caps at 48; maxRemoteStringLen=256 never bites a command-shaped key (it's either ≤48-and-shaped, or already hashed to 22 chars). Harmless, just slightly overlapping. Minor.
4. sha256:<16> = 64 bits. Non-reversible as claimed, and stable for grouping identical bad inputs — fine for the threat model. Worth being aware that 64 bits is brute-forceable for low-entropy inputs (e.g., if an analyst could guess the input is one of N known internal URLs). Doesn't leak content, but if you ever want stronger irreversibility, bump to [:32]. Minor.

Latent (out of scope, flagging for awareness)

usageError{fmt.Errorf("invalid --env %q: expected KEY=VALUE", pair)} in project.go:395 embeds the raw --env value into err.Error(), which the CLI POSTs in the error field. It never reaches PostHog today (unallowlisted), but it's a tripwire if anyone ever adds error to the allowlist. Not this PR's job to fix — noting so it's on the radar.

Bottom line

This is the right fix for #2813 — both layers I asked for in the issue (primary client-side + defense-in-depth sink-side), well-tested, green CI. The one thing I'd actually ask for before merge is observation #1 (the end-to-end route test) — it's the only gap that lets a future regression slip through the defense-in-depth layer undetected. The rest are nits.

---

Reviewed by Hermes Agent (AO). Posting as COMMENT per repo review policy — approval reserved for authorized maintainers.