Rubric conformance: error quality, retry, format flags, surface, embed, credstore - #76
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Closes several rubric gaps by improving CLI resilience (HTTP retries), error quality, output formatting (auto/styled/markdown/agent + truncation), command-surface tracking, offline skill installation via embedding, and credential storage via system keyring with fallback.
Changes:
- Added output format auto-detection plus
--agent/--styled/--markdownand--limit, with a newinternal/renderpackage for styled tables and GFM markdown rendering. - Implemented HTTP retry with exponential backoff /
Retry-Aftersupport, and improved error constructors/hints/retryability metadata. - Added surface snapshot golden file testing + Makefile targets, embedded
SKILL.md, and adoptedcredstorefor token storage/migration.
Reviewed changes
Copilot reviewed 42 out of 43 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/skills/embed_test.go | Tests ensuring embedded skill content is present and synced to canonical source. |
| internal/skills/embed.go | Embeds SKILL.md via go:embed for offline access. |
| internal/skills/SKILL.md | Embedded skill documentation content for agent/CLI usage. |
| internal/render/render_test.go | Unit tests for styled rendering helpers. |
| internal/render/render.go | Styled terminal rendering (lipgloss tables, detail views, summaries). |
| internal/render/markdown_test.go | Unit tests for markdown rendering helpers. |
| internal/render/markdown.go | GFM markdown rendering for lists/details/summaries. |
| internal/errors/errors_test.go | Expanded tests for hints and retryable markers on errors. |
| internal/errors/errors.go | Updated error constructors and HTTP-status-to-error mapping. |
| internal/config/config_test.go | Tests for HTTPS warning behavior on API URLs. |
| internal/config/config.go | Warns on insecure non-localhost http:// API URLs using net/url hostname parsing. |
| internal/commands/user.go | Switches list/detail/mutation outputs to new format-aware print helpers; adds limit/all guard. |
| internal/commands/upload.go | Uses mutation output shim for upload results. |
| internal/commands/tag.go | Uses paginated list output shim; adds limit/all guard. |
| internal/commands/surface_test.go | Golden-file test ensuring SURFACE.txt matches Cobra command tree. |
| internal/commands/step.go | Uses detail/mutation output shims (incl. location follow-up). |
| internal/commands/skill.go | Uses embedded skill content; blocks non-interactive/machine output mode. |
| internal/commands/setup.go | Blocks non-interactive/machine output mode; integrates credstore token saving behavior. |
| internal/commands/search.go | Uses paginated list output shim; adds limit/all guard. |
| internal/commands/root.go | Adds new global flags, output auto-resolution, truncation, render dispatch, credstore init/migration, agent help plumbing. |
| internal/commands/reaction.go | Uses list/mutation output shims. |
| internal/commands/pin.go | Uses list output shim. |
| internal/commands/notification.go | Uses list/paginated list/mutation output shims; adds limit/all guard. |
| internal/commands/migrate.go | Uses mutation output shim for migration results. |
| internal/commands/identity.go | Uses detail output shim. |
| internal/commands/gen_surface_test.go | Test helper for regenerating SURFACE.txt snapshot under env-gated generation. |
| internal/commands/format_test.go | Updates/extends format resolution tests incl. agent/styled/markdown/limit behavior. |
| internal/commands/comment_attachment.go | Uses list/mutation output shims for comment attachment flows. |
| internal/commands/comment.go | Uses paginated list/detail/mutation output shims; adds limit/all guard. |
| internal/commands/commands.go | Adds fizzy commands catalog and structured --help JSON behavior under agent mode. |
| internal/commands/columns.go | Central column definitions for styled/markdown table rendering. |
| internal/commands/column.go | Uses list/detail/mutation output shims. |
| internal/commands/card.go | Uses paginated list/detail/mutation output shims; adds limit/all guard. |
| internal/commands/board.go | Uses paginated list/detail/mutation output shims; adds limit/all guard. |
| internal/commands/auth_test.go | Adds credstore/token migration tests and status fields. |
| internal/commands/auth.go | Switches login/logout/status to credstore + format-aware output shims. |
| internal/commands/attachment.go | Uses list/mutation output shims for card attachments and downloads. |
| internal/client/client_test.go | Adds deterministic retry tests and Sleeper injection coverage. |
| internal/client/client.go | Implements retry/backoff with Retry-After, Sleeper injection, and improved HTTP error parsing. |
| go.sum | Adds checksums for newly introduced dependencies (keyring/tty/render). |
| go.mod | Adds direct deps for lipgloss/isatty/pflag and indirect deps for keyring/credstore support. |
| SURFACE.txt | Adds/updates surface snapshot for command/flag compatibility tracking. |
| Makefile | Adds surface-snapshot, surface-check, and sync-skill targets. |
Comments suppressed due to low confidence (1)
internal/errors/errors.go:97
- PR description mentions mapping 502/503/504 to retryable server errors, but
FromHTTPStatuscurrently routes all 5xx responses through the default case, which doesn’t setRetryable. If 5xx should be marked retryable (at least 502/503/504), add explicit cases (or a shared constructor) that setsRetryableaccordingly.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![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: da095e2146
ℹ️ 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".
github-actions
Bot
added
bug
…ructors - Add Hint field to auth errors (FromHTTPStatus 401) with fizzy-specific guidance - Mark 429 rate-limit errors as Retryable via output.ErrRateLimit - Add 502/503/504 handling in FromHTTPStatus for gateway errors - Add test coverage for hint and retryable properties
- Add doWithRetry wrapping all HTTPClient.Do callsites (request, PatchMultipart, UploadFile, UploadFileMultipart, DownloadFile) - Retry 429 responses with Retry-After header parsing (seconds or HTTP-date) - Retry 5xx and network errors with exponential backoff (1s, 2s, 4s) - Only retry POST/PATCH on 429 (explicit rate-limit), not on 5xx - Add Sleeper field for deterministic testing without real delays - Parse Retry-After into output.ErrRateLimit for structured error reporting - Tests: httptest mock verifying retry on 429/5xx, backoff durations, header parsing
…d FIZZY_DEBUG Format flags (--agent, --styled, --markdown, --limit): - Default to FormatAuto (TTY→styled, pipe→JSON) instead of FormatJSON - Add --agent (defaults to quiet), --styled, --markdown format flags - Add --limit for client-side truncation with truncation notices - Add IsMachineOutput() guard for interactive commands (setup, skill) - Add FIZZY_DEBUG env var for verbose output Render package (internal/render/): - Styled terminal output with lipgloss tables and colored key-value pairs - Markdown output with GFM tables and **key**: value pairs - Column definitions for each resource type Command output: - All list commands dispatch through printList/printListPaginated - All show commands dispatch through printDetail - All mutation commands dispatch through printMutation/printMutationWithLocation - Paginated commands pass --all flag for proper truncation notices Agent help: - --help --agent emits structured JSON (command name, flags, subcommands) - `fizzy commands` emits flat catalog of all commands
- Generate SURFACE.txt via surface.SnapshotString(rootCmd) for breaking change detection - TestSurfaceSnapshot fails when snapshot drifts with instructions to regenerate - Add Makefile targets: surface-snapshot (regenerate) and surface-check (CI validation)
- Replace runtime HTTP download of SKILL.md with go:embed in internal/skills/ - TestSkillContentFresh ensures embedded copy stays in sync with canonical source - Add Makefile sync-skill target to copy skills/fizzy/SKILL.md → internal/skills/ - Warn on stderr when API URL uses HTTP for non-localhost hosts (net/url parsing handles IPv6 loopback with port correctly)
Use basecamp/cli/credstore to store API tokens in the system keyring (macOS Keychain, GNOME Keyring, Windows Credential Manager) with automatic fallback to encrypted file storage. Token resolution precedence: YAML config → credstore → env var → flag. Existing YAML tokens auto-migrate to credstore on first load. FIZZY_NO_KEYRING=1 disables keyring and uses file fallback. Auth commands (login/logout/status) and setup wizard updated to use credstore when available. Legacy raw-string credstore entries handled with backward-compatible fallback.
jeremy
force-pushed
the
rubric-conformance
branch
from
March 5, 2026 07:25
4dd0151 to
23034c5
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.
Summary
Six atomic commits closing rubric gaps across error handling, resilience, output formatting, compatibility tracking, build hygiene, and credential security. Each commit builds and passes
makeindependently.Commits
Error quality — Shared constructors (
NewNetworkError,NewAuthError) with hints andRetryablemarkers. 502/503/504 mapped to retryable server errors.HTTP retry — Exponential backoff on 5xx/network errors,
Retry-Afterheader support on 429. InjectedSleeperfor deterministic tests. All 5HTTPClient.Docallsites covered.Format flags + render —
FormatAutodefault (TTY→styled, pipe→JSON).--agent,--styled,--markdownflags. Newinternal/render/package with lipgloss tables and GFM markdown.--limit Ntruncation with notices.FIZZY_DEBUGverbose mode.fizzy commandscatalog +--help --agentstructured JSON. Interactive commands (setup,skill) guarded against non-TTY.Surface snapshot —
SURFACE.txtviabasecamp/cli/surface.TestSurfaceSnapshotgolden-file test.make surface-snapshot/make surface-checktargets.SKILL.md embed —
go:embedreplaces runtime HTTP fetch.TestSkillContentFreshguards sync between canonical and embedded copies.make sync-skilltarget. HTTPS enforcement warning on non-localhost HTTP API URLs (usingnet/urlfor correct IPv6 parsing).Credstore adoption —
basecamp/cli/credstorefor system keyring storage with file fallback. Token precedence: YAML → credstore → env → flag. Auto-migration from YAML on first load.FIZZY_NO_KEYRING=1escape hatch. Backward-compatible legacy token parsing.Rubric criteria addressed
fizzy commandscatalogTest plan
makepasses (fmt-check, vet, lint, test-unit, tidy-check) — 0 issuesmake surface-checkpassesmakeindependentlymake test-e2ewith API credentials