feat: standardize mail +triage output to ok/data envelope#1830
Open
zhaojunlin0405 wants to merge 16 commits into
Open
feat: standardize mail +triage output to ok/data envelope#1830zhaojunlin0405 wants to merge 16 commits into
zhaojunlin0405 wants to merge 16 commits into
Conversation
zhaojunlin0405
added
enhancement
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughMail triage now defaults to JSON envelopes, supports additional output formats, exposes pagination metadata under ChangesMail output formats
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
zhaojunlin0405
force-pushed
the
feat/mail-default-json-output
branch
from
July 9, 2026 09:10
e1d151f to
c02fa92
Compare
zhaojunlin0405
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@shortcuts/mail/mail_watch.go`:
- Line 102: Add a watch test for the default format behavior in
mail_watch_test.go by exercising the +watch flow without passing --format and
asserting stdout uses the default json envelope with {"ok": true, "data": ...}.
Reuse the existing watch/test helpers around the +watch command and the relevant
mail watch entrypoints so the new case verifies the default format path in
mail_watch.go when no format flag is provided.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 17482e69-76bd-4c4d-bb8d-0f11add5381a
📒 Files selected for processing (6)
shortcuts/mail/mail_read_help_test.goshortcuts/mail/mail_triage.goshortcuts/mail/mail_triage_test.goshortcuts/mail/mail_watch.goskills/lark-mail/references/lark-mail-triage.mdskills/lark-mail/references/lark-mail-watch.md
✅ Files skipped from review due to trivial changes (2)
- skills/lark-mail/references/lark-mail-watch.md
- skills/lark-mail/references/lark-mail-triage.md
🚧 Files skipped from review as they are similar to previous changes (2)
- shortcuts/mail/mail_read_help_test.go
- shortcuts/mail/mail_triage_test.go
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1830 +/- ##
==========================================
+ Coverage 74.58% 74.67% +0.08%
==========================================
Files 861 877 +16
Lines 89971 91736 +1765
==========================================
+ Hits 67106 68503 +1397
- Misses 17669 17922 +253
- Partials 5196 5311 +115 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@81b778de692e20411ea504390e67c876a96679bd🧩 Skill updatenpx skills add larksuite/cli#feat/mail-default-json-output -y -g |
zhaojunlin0405
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/output/envelope.go`:
- Around line 17-22: Remove `omitempty` from the `Meta.Count` JSON tag so zero
counts remain serialized, and update the empty-list envelope test to assert that
`count` is present with value zero.
In `@shortcuts/mail/mail_json_shorthand_test.go`:
- Around line 96-110: Update TestMailTriageRejectsRemovedDataFormat to assert
the typed validation metadata returned by errs.ProblemOf, matching
TestMailTriageEnumRejectsUnknownFormat: verify the expected Category, Subtype,
and Param values for the rejected --format input, and remove the
message-substring assertion.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5cc992be-b575-4a3a-8eda-a71be3c71e4c
📒 Files selected for processing (8)
internal/output/envelope.gointernal/output/envelope_test.goshortcuts/mail/mail_json_shorthand_test.goshortcuts/mail/mail_read_help_test.goshortcuts/mail/mail_triage.goshortcuts/mail/mail_triage_test.goskills/lark-mail/references/lark-mail-triage.mdskills/lark-mail/references/lark-mail-watch.md
💤 Files with no reviewable changes (1)
- shortcuts/mail/mail_read_help_test.go
✅ Files skipped from review due to trivial changes (2)
- skills/lark-mail/references/lark-mail-watch.md
- skills/lark-mail/references/lark-mail-triage.md
Align TestMailTriageRejectsRemovedDataFormat with the repo *_test.go guideline (error-path tests assert typed errs.ProblemOf metadata — category/subtype/param — not message substrings alone), matching the sibling TestMailTriageEnumRejectsUnknownFormat. Addresses CodeRabbit review on PR #1830.
The default json watch stream emits one enveloped JSON object per line via
PrintNdjson, but the fetch-message failure branch used PrintJson (multi-line
MarshalIndent) and bypassed the envelope, so a mid-stream failure injected a
multi-line {ok:false,...} blob that breaks line-based NDJSON parsers. Route
failures through watchFailureOutputValue: bare payload for --format data,
identity-tagged single line for the default json envelope.
Meta.Count had json:"count,omitempty", so an empty mail +triage result dropped .meta.count entirely, contradicting the migration contract (.count -> .meta.count) and forcing callers to distinguish missing-vs-zero. Every output.Meta construction already sets Count=len(data), so dropping omitempty only makes empty list envelopes emit a correct count:0 across all list commands. Add empty-result and serialization assertions.
Align mail +triage with the repo-wide list convention: pagination lives inside
the data object, not meta. Output is {ok, data:{messages, total, has_more,
page_token}} with meta=nil.
Uses a typed triageOutput struct (like calendar +search-event) rather than a
map: output.toGeneric JSON-round-trips a struct, normalizing the nested
messages into []interface{} so output.ExtractItems can find it for
table/csv/ndjson rendering. (A top-level map is not round-tripped, which would
flatten the wrapper into a single row — caught by the new
TestMailTriageNdjsonEmitsOneMessagePerLine regression test.)
internal/output/envelope.go is restored to main (no Meta.HasMore/PageToken, no
Count omitempty change) — this PR no longer touches the shared envelope struct;
supersedes the CodeRabbit-A/P2 discussion. Docs, skill-eval, E2E updated to
.data.messages / .data.total / .data.has_more / .data.page_token.
…ON line
Two review follow-ups on mail +watch:
- --print-output-schema showed only the bare per-event structure (.message
paths), but the default --format json wraps each event as
{ok,identity,data:<structure>}. Callers building parse logic from the schema
would look for .message and miss it (it is at .data.message under the default).
Add a _format_note documenting the .data prefix for default json.
- The NDJSON failure test only json.Marshal'd watchFailureOutputValue and never
went through the real print function, so a revert to multi-line PrintJson
would not be caught. Add TestWatchFailurePrintsSingleNDJSONLine, which drives
output.PrintNdjson (the function the failure branch calls) and asserts exactly
one decodable line for both json and data formats.
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
1 participant
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
mail +triagehistorically printed a bare JSON object, inconsistent with therest of the CLI. This standardizes it onto the
{ok, data}envelope, with themessage list and pagination inside
data— matching the repo-wide listconvention (
im +chat-messages-list,calendar +search-event, etc.):{"ok":true,"data":{"messages":[{…,"mailbox_id":"me"}],"total":N,"has_more":true,"page_token":"list:…"}}--formatsupportsjson | pretty | table | ndjson | csv(Enum-validated,default
json;--jsonshorthand);table/csv/ndjsonrender the message rowsvia
output.ExtractItems,prettyrenders the curated human table.mail +watchkeeps its streaming output but its default--formatis nowjson,and its fetch-failure path is emitted as a single NDJSON line (no longer a
multi-line blob). This is a breaking change to
mail +triage's JSON contract(migration below).
Changes
mail +triageoutput through a typedtriageOutputstruct →{ok, data:{messages,total,has_more,page_token}},metanil(
shortcuts/mail/mail_triage.go). Uses a struct (likecalendar +search-event) sooutput.toGenericround-trips it andExtractItemscanextract
messagesfor table/csv/ndjson.mail +watch: default--format→json; keep the fetch-failure line on asingle NDJSON line via
watchFailureOutputValue(shortcuts/mail/mail_watch.go)..data.messages/.data.total|has_more|page_token;add an ndjson-per-line regression test and a typed-error
--format datarejection test.
skills/lark-mail/references/lark-mail-triage.md; updatelark-mail-watch.mddefault.internal/output/envelope.gois unchanged — pagination lives indata,so no shared
Metafields were added.Test Plan
go build ./...,go test ./shortcuts/mail/... ./internal/output/...--format datarejection,--jsonequivalence, ndjson per-line)
mail +watchdefault enveloped-NDJSON stream — not provable in abounded E2E (long-running WebSocket)
Breaking change (
mail +triage):.messages→.data.messages;.count→.data.total;.has_more/.page_token→.data.has_more/.data.page_token;top-level
mailbox_idremoved (still per message);--format dataremoved(rejected by the Enum); the curated table moved from
--format tableto--format pretty.Related Issues
N/A