Conversation
renderObject unconditionally skipped the "comments" key, then topLevelComments tried to reconstruct it as []map[string]any. When comments was a different shape (string, int, []string) — as happens with `basecamp api get` — reconstruction returned nil and data was silently dropped. Only skip "comments" in the field loop when topLevelComments returns non-nil. Both styled and markdown renderers.
TestStyledRenderObjectPreservesUnknownAttachmentFields asserted custom_attachments_report survived — but that key doesn't end with _attachments, so it passed under the old code too. previewable_attachments was in the test data but never asserted. Assert previewable_attachments values appear in output (both styled and markdown). Remove the misleading assertion.
Three mutually exclusive flags: --comments / --no-comments / --all-comments. addCommentFlags(cmd, defaultOn) controls whether comments are fetched by default (show) or require opt-in (typed show commands). New fetchCommentsForRecording derives count from API response metadata, not the parent's comments_count. fetchRecordingComments becomes a thin wrapper that falls back to the parent's count when Meta.TotalCount is unavailable.
Drop the comments_count gate — basecamp show now always attempts comment fetching for commentable types. Skip the fetch for people and boosts which don't support comments. Update tests: expect the extra comments request, add X-Total-Count header support to mock transport, rename TestShowCommentsMissingField to TestShowSkipsCommentsForNonCommentableTypes.
Wire fetchCommentsForRecording into all commentable show commands: todos, messages, cards, files, todolists, schedule (both paths), checkins (question + answer), forwards, chat line. defaultOn=false means typed commands require --comments or --all-comments to opt in. Attachment notices are merged via applyNotices. Comments on comments are excluded (flat model).
Regenerate CLI surface snapshot with new --comments, --no-comments, --all-comments flags on typed show commands. Acknowledge comments show flag removal in .surface-breaking. Update skill with typed show command examples.
github-actions
bot
added
commands
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 18 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name=".surface">
<violation number="1" location=".surface:4062">
P2: `basecamp comments show` should not expose comment-fetch flags; comments-on-comments are excluded in the flat model.</violation>
</file>
<file name="internal/output/render.go">
<violation number="1" location="internal/output/render.go:777">
P2: Using `len(comments) > 0` to decide whether to hide the `comments` field fails for empty comment arrays, so `comments` leaks into detail output as a regular field.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
![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: dda6ad1b9b
ℹ️ 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".
There was a problem hiding this comment.
Pull request overview
This PR extends the CLI’s “show” experience by standardizing comment-fetching behavior: basecamp show now fetches comments by default for commentable types, and typed … show commands gain a consistent --comments / --no-comments / --all-comments flag model. It also includes a renderer bug fix around top-level comments filtering and updates tests/docs/surface snapshots accordingly.
Tip
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or run gh pr ready --undo.
Click "Ready for review" or run gh pr ready to reengage.
Changes:
- Add a three-flag comment model (
--comments / --no-comments / --all-comments) and wire it into multiple typedshowcommands. - Make
basecamp showfetch comments by default (with type gating for non-commentable recordings). - Adjust object rendering/tests/docs and update
.surfacesnapshots to reflect the new CLI surface.
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| skills/basecamp/SKILL.md | Documents updated show/comment-flag behavior. |
| internal/output/render.go | Adjusts when the raw comments field is skipped vs rendered. |
| internal/output/output_test.go | Updates renderer regression coverage around attachment fields. |
| internal/commands/todos.go | Wires comment enrichment + notice merging into todos show. |
| internal/commands/todolists.go | Wires comment enrichment into todolists show. |
| internal/commands/show_test.go | Updates show tests for “always fetch comments” and new count/headers behavior. |
| internal/commands/show.go | Adds type gating for comment fetch and updates comment flag defaults for basecamp show. |
| internal/commands/schedule.go | Wires comment enrichment into schedule show. |
| internal/commands/messages.go | Wires comment enrichment + notice merging into messages show. |
| internal/commands/forwards.go | Wires comment enrichment into forwards show. |
| internal/commands/files.go | Wires comment enrichment + notice merging into files show. |
| internal/commands/comments_embed_test.go | Updates tests for default-on vs opt-in comment flag behavior. |
| internal/commands/comments_embed.go | Implements the three-flag model and splits typed-vs-map comment fetching helpers. |
| internal/commands/checkins.go | Wires comment enrichment into checkins … show (question/answer). |
| internal/commands/chat.go | Wires comment enrichment into chat line (and alias show). |
| internal/commands/cards.go | Wires comment enrichment + notice merging into cards show. |
| .surface-breaking | Records CLI-surface breaking changes related to newly added flags. |
| .surface | Updates the CLI surface snapshot to include the new comment flags across commands. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Use isCommentsArray to decide whether to suppress the raw
comments field, not len(comments) > 0. topLevelComments returns
nil for empty arrays (toMapSlice on []any{}), so empty comments
arrays were rendered as regular fields instead of being suppressed.
Rename attachment tests to reflect updated fixture focus.
Add comment/comments CLI aliases and Comment API type to isCommentableShowType's non-commentable list. Comments are flat — attempting to fetch comments-on-comments wastes a request and can produce misleading diagnostics.
Register --comments/--no-comments/--all-comments on the parent question and answer commands so both entry points (shortcut and explicit show) accept the same flags.
Fresh surface snapshot reflects comment type exclusion and checkins parent flag wiring. Remove the comments show flag removal entries from .surface-breaking — the flags were never in the regenerated baseline.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3f6f5d1fca
ℹ️ 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".
| case "people", "boosts", "comment", "comments": | ||
| return false |
There was a problem hiding this comment.
isCommentableShowType only filters out people, boosts, and comment, so basecamp show now treats other non-commentable recordings (for example Vaults, whose payloads in e2e/cassettes/happypath/cassette.json include no comments_url/comments_count) as commentable. Because show now defaults to fetching comments, these types incur an unnecessary /recordings/{id}/comments request and can surface a misleading “Comment fetching failed” diagnostic on normal lookups.
Useful? React with 👍 / 👎.
| data = withComments(data, enrichment.Comments) | ||
| opts = append(opts, enrichment.applyNotices(attachmentNotice)...) |
There was a problem hiding this comment.
This command now fetches comments and injects them into data, but it still renders via WithEntity("message"), which routes styled/markdown output through the message presenter schema; that schema does not render a comments section. In practice, basecamp messages show --comments makes the extra API call yet default human output still omits the fetched comments, so the new flag appears ineffective outside JSON.
Useful? React with 👍 / 👎.
Summary
Follow-up to #389 (merged). Fixes two bugs found in review, makes
basecamp showalways fetch comments (dropping thecomments_countgate), and adds--comments/--no-comments/--all-commentsto all commentable typed show commands.renderObjectunconditionally skipped thecommentskey, causing data loss when comments was a non-array shape (string, int) viabasecamp api get. Now only skips whentopLevelCommentsreturns non-nil._attachmentsregression test asserted a key that was never filtered. Replaced withpreviewable_attachmentswhich actually exercises the filter.--comments/--no-comments/--all-comments(mutually exclusive).defaultOn=trueforshow,falsefor typed commands (opt-in).basecamp showno longer requirescomments_countin the response. Non-commentable types (people, boosts) are gated.Test plan
bin/cipasses cleanbasecamp show <any-commentable-id>fetches comments by default — 2 requests, comments key present, summary includes countbasecamp show <person-id>does NOT attempt comment fetch — 1 request, no comments keybasecamp todos show <id> --commentsfetches comments — 2 requests, comments presentbasecamp cards show <id> --all-commentsfetches all comments — 2 requestsbasecamp messages show <id> --no-commentssuppresses comments — 1 request, no comments keybasecamp comments show <id>has no comment flags (flat model) — help shows no --comments/--no-comments/--all-comments