Conversation
![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: 8c4c97d978
ℹ️ 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".
Contributor
There was a problem hiding this comment.
Pull request overview
This PR migrates the Fizzy CLI from the local internal/response envelope to the shared github.com/basecamp/cli/output package, adds mutually-exclusive global output format flags, and updates unit/E2E tests + docs to match the new envelope (ok/error + context).
Changes:
- Replace
internal/responsewithoutput.Writer+ sharedoutput.Response/output.Errortypes and conventions. - Add global output format flags (
--json,--quiet,--ids-only,--count) resolved inPersistentPreRunE. - Update docs/tests and align exit-code mappings to the shared rubric; change default
maketarget tocheck.
Reviewed changes
Copilot reviewed 66 out of 67 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| skills/fizzy/SKILL.md | Update flags + envelope docs |
| internal/response/response.go | Remove legacy response package |
| internal/errors/errors.go | Re-export shared output errors/codes |
| internal/errors/errors_test.go | Update exit-code/error-code tests |
| internal/commands/root.go | Add format flags + output.Writer shims |
| internal/commands/format_test.go | Add format flag tests |
| internal/commands/version.go | Convert to RunE + return errors |
| internal/commands/board.go | Migrate to RunE + output helpers |
| internal/commands/board_test.go | Update tests for RunE/output |
| internal/commands/column.go | Migrate to RunE + output helpers |
| internal/commands/column_test.go | Update tests for RunE/output |
| internal/commands/comment.go | Migrate to RunE + output helpers |
| internal/commands/comment_test.go | Update tests for RunE/output |
| internal/commands/comment_attachment.go | Migrate to RunE + output helpers |
| internal/commands/comment_attachment_test.go | Update tests for RunE/output |
| internal/commands/reaction.go | Migrate to RunE + output helpers |
| internal/commands/reaction_test.go | Update tests for RunE/output |
| internal/commands/pin.go | Migrate to RunE + output helpers |
| internal/commands/pin_test.go | Update tests for RunE/output |
| internal/commands/notification.go | Migrate to RunE + output helpers |
| internal/commands/notification_test.go | Update tests for RunE/output |
| internal/commands/user.go | Migrate to RunE + output helpers |
| internal/commands/user_test.go | Update tests for RunE/output |
| internal/commands/upload.go | Migrate to RunE + output helpers |
| internal/commands/upload_test.go | Update tests for RunE/output |
| internal/commands/tag.go | Migrate to RunE + output helpers |
| internal/commands/tag_test.go | Update tests for RunE/output |
| internal/commands/step.go | Migrate to RunE + output helpers |
| internal/commands/step_test.go | Update tests for RunE/output |
| internal/commands/search.go | Migrate to RunE + output helpers |
| internal/commands/search_test.go | Update tests for RunE/output |
| internal/commands/identity.go | Migrate to RunE + output helpers |
| internal/commands/identity_test.go | Update tests for RunE/output |
| internal/commands/migrate.go | Migrate to RunE + output helpers |
| internal/commands/migrate_test.go | Update tests for RunE/output |
| internal/commands/auth.go | Migrate to RunE + output helpers |
| internal/commands/auth_test.go | Update tests for RunE/output |
| internal/commands/attachment.go | Migrate to RunE + output helpers |
| internal/commands/attachment_test.go | Update tests for RunE/output |
| internal/commands/setup.go | Convert setup wizard to RunE |
| internal/commands/skill.go | Convert skill installer to RunE |
| internal/commands/mock_client_test.go | Add assertExitCode helper |
| internal/client/client_test.go | Update error expectations/methods |
| e2e/harness/harness.go | Update harness envelope + exit codes |
| e2e/tests/auth_test.go | Update to ok/error envelope |
| e2e/tests/board_test.go | Update to ok/context pagination |
| e2e/tests/card_test.go | Update to ok/error envelope |
| e2e/tests/column_test.go | Update to ok/error envelope |
| e2e/tests/comment_test.go | Update to ok/error envelope |
| e2e/tests/comment_attachment_test.go | Update to ok/error envelope |
| e2e/tests/error_test.go | Update error envelope assertions |
| e2e/tests/identity_test.go | Update error shape (code/error string) |
| e2e/tests/notification_test.go | Update to ok/error envelope |
| e2e/tests/pin_test.go | Update to ok/error envelope |
| e2e/tests/reaction_test.go | Update to ok/error envelope |
| e2e/tests/step_test.go | Update to ok/error envelope |
| e2e/tests/tag_test.go | Update to ok/error envelope |
| e2e/tests/upload_test.go | Update to ok/error envelope |
| e2e/tests/user_test.go | Update to ok/error envelope |
| README.md | Update JSON envelope example |
| Makefile | Default target -> check |
| go.mod | Add github.com/basecamp/cli dep |
| go.sum | Update module checksums |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Replace the internal response package with basecamp/cli/output, which provides a standard CLI envelope (ok/error), structured error types, and an output.Writer for format-aware rendering. - Rewrite internal/errors as thin type aliases over output.Error - Wire output.Writer into root command Execute() and test harness - Update all command files to use Breadcrumb alias from root.go - Update all unit tests for .Response.OK (was .Response.Success), method-based .ExitCode(), and Context-based location/pagination - Delete internal/response/ (superseded by output package)
Rewrite the e2e harness Response struct to match the output package envelope (OK/Error fields, Context for location/pagination). Update all 16 e2e test files for the new field names and assertion helpers.
Reflect the new response envelope field names (ok/error instead of success/failure) in the SKILL.md agent documentation and README.
Replace the dead --pretty flag with four output format flags that thread through the shared output package: --json JSON envelope (default, explicit) --quiet Raw JSON data without envelope --ids-only One ID per line --count Integer count of results Format resolution happens in PersistentPreRunE after Cobra parses flags. At most one format flag may be set; conflicts produce a usage error. Default remains FormatJSON — FormatAuto deferred until the output package has renderer support.
- Check return values from out.OK(), out.Err(), json.Unmarshal (errcheck) - Add nolint:nilerr directives for intentional nil returns on user cancellation in setup and skill interactive prompts
make with no arguments now runs the full CI gate (fmt-check + vet + lint + test-unit + tidy-check) instead of printing help.
- Wrap plain errors in auth, skill, and completion commands as
output.Error{Code: CodeAPI} so they aren't misclassified as usage
errors by the Execute() catch-all.
- Only set lastResult.Response on successful JSON unmarshal in
captureResponse, avoiding zero-value responses for non-JSON formats.
- Reset format flag variables in runCobraWithArgs to prevent state
leaking between integration test calls.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 67 out of 68 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Clear lastResult.Response before unmarshal attempt in captureResponse
to prevent stale responses leaking across non-JSON format test runs.
- Use any instead of interface{} in printSuccess helper signatures and
pagination context map for consistency with repo conventions.
- Preserve server-provided message on 429 rate limit errors in
FromHTTPStatus instead of dropping it.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 67 out of 68 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Data comes back as []any from JSON unmarshal, not []Attachment. The previous assertion silently failed, skipping the count check.
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
internal/responsewithbasecamp/cli/outputpackage for standardized CLI envelope (ok/error), structured error types, and format-aware output--json,--quiet,--ids-only,--countglobal format flags (mutually exclusive, default JSON)--prettyflagmaketarget tocheck(CI gate)Commits
--json/--quiet/--ids-only/--countwith PersistentPreRunE resolution, unit + Cobra integration testshelp→checkFormat flags
--jsonok,data,breadcrumbs,context(default)--quiet--ids-only--countConflicting flags produce a usage error. Format resolution happens in
PersistentPreRunEafter Cobra parses flags.Test plan
make(check) passes — 0 lint issues, all unit tests greengo run ./cmd/fizzy version --count→1go run ./cmd/fizzy version --quiet→ raw JSON, no envelopego run ./cmd/fizzy version --quiet --count→ usage error, exit 1make test-e2e(needs credentials)