Skip to content

I12 (audit-I3+I20+I22+I24+I38+I40): input validation polish redux#41

Merged
mfwolffe merged 5 commits into
trunkfrom
i12/input-validation-redux
May 25, 2026
Merged

I12 (audit-I3+I20+I22+I24+I38+I40): input validation polish redux#41
mfwolffe merged 5 commits into
trunkfrom
i12/input-validation-redux

Conversation

@espadonne
Copy link
Copy Markdown
Contributor

Summary

Closes the six remaining input-validation gaps the I-audit caught after H6/H7/H10's first pass.

What landed

  • `api --hostname ""` rejects (audit-I3, MED) — H7 closed whitespace + scheme; the bare-empty case still silently fell back to the active host. Added `HostnameSet` so cobra's `Flags().Changed("hostname")` distinguishes "user typed --hostname" from "user didn't pass it." Explicit empty now hits the same "config: hostname is required" error.
  • `api --cache` clamp + wrapped error (audit-I20+I22+I38, MED/PERF) — capped at `maxCacheTTL = 24h` (gh's documented cap). Negative + zero values rejected. Parse error wrapped with an actionable example string (`5m, 1h, 24h`) so the Go-internal `time: invalid duration` prefix stops leaking through.
  • `repo create` slug validation (audit-I24, MED) — `repo create "../sneaky"` used to surface "org not found" (confusing) or hit the server's slug regex with a less helpful message. `validSlug` now rejects bad owner/repo segments at the CLI boundary with a clear "invalid owner name" / "invalid repo name" error naming the bad component. Character class matches what the server allows: `[a-zA-Z0-9._-]`, no leading/trailing punctuation.
  • `repo edit --remove-topic` empty-repo warning (audit-I40, MIN) — H28's note ("topic X was not present") was already wired; the audit observation was stale. Added a regression test for the empty-topics-list case so the warning doesn't quietly regress.

Out of scope

  • The audit's reproducer for I40 reported "still broken" — verified the code path works; flagging as audit observation drift. The new test pins the empty-list case.

Test plan

  • Build + vet clean
  • Targeted regression tests pass: `TestRunHostnameExplicitEmptyRejects`, `TestRunCacheRejectsOverCap`, `TestRunCacheRejectsZeroOrNegative`, `TestRunCacheWrapsParseError`, `TestSplitNameArgRejectsSneakyNames`, `TestSplitNameArgAcceptsGoodForms`, `TestEditRemoveTopicEmptyRepoStillWarns`
  • Full `go test ./...` green
  • Dogfood post-merge: `shithub api --hostname "" /user` errors; `shithub api --cache 999999h /user` errors; `shithub repo create "../sneaky"` errors with the new "invalid owner name" message

🤖 Generated with Claude Code

@mfwolffe mfwolffe merged commit 89400f2 into trunk May 25, 2026
3 checks passed
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.

2 participants