fix: align api success envelopes#1489
Merged
liangshuo-1 merged 1 commit intoJun 17, 2026
Merged
Conversation
|
|
github-actions
Bot
added
the
size/L
📝 WalkthroughWalkthroughIntroduces ChangesSuccess Envelope, Content-Safety Pagination, and StreamPages Error Propagation
Sequence Diagram(s)sequenceDiagram
participant CLI as apiPaginate / servicePaginate
participant PaginateAll
participant StreamPages
participant ScanForSafety
participant WriteSuccessEnvelope
alt --jq set
CLI->>PaginateAll: request, pagOpts
PaginateAll-->>CLI: aggregated result
CLI->>WriteSuccessEnvelope: result, {CommandPath, Identity, JqExpr}
WriteSuccessEnvelope->>ScanForSafety: aggregated data
ScanForSafety-->>WriteSuccessEnvelope: alert or block error
WriteSuccessEnvelope-->>CLI: jq-filtered envelope JSON
else streaming format (ndjson/table/csv)
CLI->>StreamPages: request, onItems callback, pagOpts
loop per page
StreamPages->>ScanForSafety: page items, commandPath, errOut
alt blocked
ScanForSafety-->>StreamPages: block error
StreamPages-->>CLI: halt pagination
else warn/ok
ScanForSafety-->>StreamPages: alert (optional)
StreamPages-->>CLI: format page items to stdout
end
end
else default JSON
CLI->>PaginateAll: request, pagOpts
PaginateAll-->>CLI: aggregated result
CLI->>WriteSuccessEnvelope: result, {CommandPath, Identity}
WriteSuccessEnvelope->>ScanForSafety: aggregated data
ScanForSafety-->>WriteSuccessEnvelope: alert or block error
WriteSuccessEnvelope-->>CLI: envelope JSON
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
internal/client/client_test.go (1)
110-192:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd a dedicated error-path test for the new callback contract.
These updates only adapt callback signatures, but they don’t verify the new behavior:
onItemsreturning an error should halt pagination and propagate that error fromStreamPages. Please add a test that returns a sentinel error fromonItemsand asserts both propagation and early stop semantics.As per coding guidelines, “Every behavior change needs a test alongside the change.”
🤖 Prompt for 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. In `@internal/client/client_test.go` around lines 110 - 192, Add a new test function that verifies the error-handling behavior of the StreamPages method when the onItems callback returns an error. Create a test that configures a mock response with multiple pages or items, then have the onItems callback return a sentinel error (for example, a custom error or errors.New("test error")), and assert that StreamPages propagates that error and halts pagination early (confirming that not all items were processed). This test should demonstrate both the error propagation and the early-stop semantics of the callback contract.Source: Coding guidelines
cmd/api/api_test.go (1)
358-390:⚠️ Potential issue | 🟠 Major | ⚡ Quick winStrengthen typed error assertions for the newly added API error paths.
Line 388 only verifies that the error is typed, and Line 622 still validates behavior via message substring. For these error-path tests, please assert typed metadata (
category/subtype, andparamwhere applicable) plus cause preservation, not just presence/string checks.As per coding guidelines,
*_test.goerror-path tests must assert typed metadata viaerrs.ProblemOfand cause preservation rather than message substrings. Based on learnings, when assertingparam, useerrors.As(..., *errs.ValidationError)sinceerrs.ProblemOfdoes not exposeParam.Also applies to: 583-624
🤖 Prompt for 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. In `@cmd/api/api_test.go` around lines 358 - 390, The error-path tests in TestApiCmd_PageAll_NonBatchAPI_ErrorStillOutputsJSON and the test at lines 583-624 are using weak assertions that only check for error presence and message substrings instead of verifying typed error metadata and cause preservation. Replace the simple typed error checks (using errs.ProblemOf for presence only) and string substring validations with assertions that verify the complete typed metadata including category, subtype, and param fields where applicable. When asserting the param field, use errors.As to extract the ValidationError type directly since errs.ProblemOf does not expose Param, and ensure cause preservation is also validated as part of these assertions.Sources: Coding guidelines, Learnings
cmd/service/service.go (1)
666-668:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMissing raw JSON output on business error paths in
servicePaginate. Both the streaming and default branches omit theoutput.FormatValue(out, result, output.FormatJSON)call before returning a business error, unlike the jq path in this file (lines 633-636) and all three paths inapi.go. This prevents users from inspecting the actual API response when a business error occurs.
cmd/service/service.go#L666-L668: Addoutput.FormatValue(out, result, output.FormatJSON)before returningapiErrin the streaming path.cmd/service/service.go#L684-L686: Addoutput.FormatValue(out, result, output.FormatJSON)before returningapiErrin the default path.🤖 Prompt for 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. In `@cmd/service/service.go` around lines 666 - 668, The servicePaginate function in cmd/service/service.go omits raw JSON output on business error paths, preventing users from inspecting API responses when errors occur. In cmd/service/service.go at lines 666-668, add output.FormatValue(out, result, output.FormatJSON) before returning apiErr in the streaming path where checkErr returns a non-nil error. In cmd/service/service.go at lines 684-686, add the same output.FormatValue(out, result, output.FormatJSON) call before returning apiErr in the default path where checkErr returns a non-nil error. This ensures both branches output the raw JSON response before returning, consistent with the jq path and the patterns used in api.go.
🤖 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 `@cmd/service/service_test.go`:
- Around line 595-639: The error-path tests in
TestServiceMethod_PageAll_StreamFormatBlockSkipsBlockedPage (595-639), the test
at 649-675, and the test at 866-895 currently only validate error message
presence using string contains checks, but should instead validate typed error
metadata and preserved cause chains. For each affected test, replace the
message-only assertions with assertions that check the error's typed metadata
using errors.As to validate fields like category, subtype, and param when
relevant (since errs.ProblemOf only exposes problem-level fields), and verify
the cause chain is preserved by checking that the underlying error wraps the
expected root cause. Update all three test locations to follow this pattern
consistently.
---
Outside diff comments:
In `@cmd/api/api_test.go`:
- Around line 358-390: The error-path tests in
TestApiCmd_PageAll_NonBatchAPI_ErrorStillOutputsJSON and the test at lines
583-624 are using weak assertions that only check for error presence and message
substrings instead of verifying typed error metadata and cause preservation.
Replace the simple typed error checks (using errs.ProblemOf for presence only)
and string substring validations with assertions that verify the complete typed
metadata including category, subtype, and param fields where applicable. When
asserting the param field, use errors.As to extract the ValidationError type
directly since errs.ProblemOf does not expose Param, and ensure cause
preservation is also validated as part of these assertions.
In `@cmd/service/service.go`:
- Around line 666-668: The servicePaginate function in cmd/service/service.go
omits raw JSON output on business error paths, preventing users from inspecting
API responses when errors occur. In cmd/service/service.go at lines 666-668, add
output.FormatValue(out, result, output.FormatJSON) before returning apiErr in
the streaming path where checkErr returns a non-nil error. In
cmd/service/service.go at lines 684-686, add the same output.FormatValue(out,
result, output.FormatJSON) call before returning apiErr in the default path
where checkErr returns a non-nil error. This ensures both branches output the
raw JSON response before returning, consistent with the jq path and the patterns
used in api.go.
In `@internal/client/client_test.go`:
- Around line 110-192: Add a new test function that verifies the error-handling
behavior of the StreamPages method when the onItems callback returns an error.
Create a test that configures a mock response with multiple pages or items, then
have the onItems callback return a sentinel error (for example, a custom error
or errors.New("test error")), and assert that StreamPages propagates that error
and halts pagination early (confirming that not all items were processed). This
test should demonstrate both the error propagation and the early-stop semantics
of the callback contract.
🪄 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: bafc2526-b813-464a-8093-2dfd326c788b
📒 Files selected for processing (12)
cmd/api/api.gocmd/api/api_test.gocmd/service/service.gocmd/service/service_test.gointernal/client/client.gointernal/client/client_test.gointernal/client/pagination.gointernal/client/response.gointernal/client/response_test.gointernal/output/envelope_success.gointernal/output/envelope_success_test.goshortcuts/common/runner.go
💤 Files with no reviewable changes (1)
- internal/client/pagination.go
Tantanz20020918
force-pushed
the
fix/api-envelope-alignment
branch
from
June 17, 2026 03:54
0be7518 to
d0b721c
Compare
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 `@internal/output/envelope_success.go`:
- Around line 47-51: The issue is that when a jq expression filters the envelope
output, the content-safety alert can be filtered away, exposing flagged data
without any visible warning. Before calling JqFilter when opts.JqExpr is not
empty, check if scanResult.Alert exists and if so, emit a warning message to
stderr to ensure the alert is visible even when jq filters the envelope fields.
Additionally, add a regression test case that verifies the stderr warning is
emitted when both a content-safety alert is present and jq filtering is applied.
🪄 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: 806377be-e5e3-44a2-98b7-2672f4216d49
📒 Files selected for processing (12)
cmd/api/api.gocmd/api/api_test.gocmd/service/service.gocmd/service/service_test.gointernal/client/client.gointernal/client/client_test.gointernal/client/pagination.gointernal/client/response.gointernal/client/response_test.gointernal/output/envelope_success.gointernal/output/envelope_success_test.goshortcuts/common/runner.go
💤 Files with no reviewable changes (1)
- internal/client/pagination.go
🚧 Files skipped from review as they are similar to previous changes (6)
- shortcuts/common/runner.go
- internal/output/envelope_success_test.go
- cmd/service/service.go
- internal/client/response_test.go
- internal/client/client.go
- cmd/api/api.go
Tantanz20020918
force-pushed
the
fix/api-envelope-alignment
branch
from
June 17, 2026 06:09
d0b721c to
c8408b7
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/output/envelope_success_test.go (1)
55-76: ⚡ Quick winAvoid whitespace-sensitive JSON assertions in this test.
These
strings.Containschecks couple the test toPrintJsonformatting (spaces/newlines), so harmless formatting changes can fail the test. Parse JSON and assert fields structurally instead.Proposed refactor
import ( + "encoding/json" "strings" "testing" extcs "github.com/larksuite/cli/extension/contentsafety" ) @@ got := out.String() - for _, want := range []string{`"ok": true`, `"identity": "bot"`, `"data": {`, `"id": "1"`} { - if !strings.Contains(got, want) { - t.Fatalf("output missing %s:\n%s", want, got) - } - } - if strings.Contains(got, `"code"`) || strings.Contains(got, `"msg"`) { - t.Fatalf("output leaked Lark protocol fields:\n%s", got) - } - if strings.Contains(got, `_content_safety_alert`) { - t.Fatalf("output should omit empty content-safety alert:\n%s", got) - } + var env map[string]interface{} + if err := json.Unmarshal([]byte(got), &env); err != nil { + t.Fatalf("invalid json output: %v\n%s", err, got) + } + if env["ok"] != true || env["identity"] != "bot" { + t.Fatalf("unexpected envelope: %#v", env) + } + data, ok := env["data"].(map[string]interface{}) + if !ok || data["id"] != "1" { + t.Fatalf("unexpected data payload: %#v", env["data"]) + } + if _, exists := env["code"]; exists { + t.Fatalf("output leaked protocol field code: %#v", env) + } + if _, exists := env["msg"]; exists { + t.Fatalf("output leaked protocol field msg: %#v", env) + } + if _, exists := env["_content_safety_alert"]; exists { + t.Fatalf("output should omit empty content-safety alert: %#v", env) + } }🤖 Prompt for 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. In `@internal/output/envelope_success_test.go` around lines 55 - 76, The test TestWriteSuccessEnvelope_PrintsShortcutCompatibleEnvelope uses strings.Contains to verify JSON output formatting, which couples the test to whitespace and formatting decisions. Replace these string-based assertions with JSON parsing: unmarshal the output string into a map or appropriate struct, then assert on the actual field values and structure instead. This will make the test robust to harmless formatting changes while still validating that the correct fields and values are present in the envelope.
🤖 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_success_test.go`:
- Around line 96-125: Add a new test function following the pattern of
TestWriteSuccessEnvelope_JqWarnsWhenSafetyAlertFiltered to cover the blocked
safety mode path. Set the environment variable LARKSUITE_CLI_CONTENT_SAFETY_MODE
to "block" with the same mockProvider setup, then call WriteSuccessEnvelope and
verify that it returns an error rather than nil. Assert that the error has the
correct type metadata (not just string message matching) and that the cause
chain is preserved, while also verifying that the stdout output buffer remains
empty when the block mode triggers an early return.
---
Nitpick comments:
In `@internal/output/envelope_success_test.go`:
- Around line 55-76: The test
TestWriteSuccessEnvelope_PrintsShortcutCompatibleEnvelope uses strings.Contains
to verify JSON output formatting, which couples the test to whitespace and
formatting decisions. Replace these string-based assertions with JSON parsing:
unmarshal the output string into a map or appropriate struct, then assert on the
actual field values and structure instead. This will make the test robust to
harmless formatting changes while still validating that the correct fields and
values are present in the envelope.
🪄 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: 5f13e644-ace6-46db-97ef-145d6de58984
📒 Files selected for processing (12)
cmd/api/api.gocmd/api/api_test.gocmd/service/service.gocmd/service/service_test.gointernal/client/client.gointernal/client/client_test.gointernal/client/pagination.gointernal/client/response.gointernal/client/response_test.gointernal/output/envelope_success.gointernal/output/envelope_success_test.goshortcuts/common/runner.go
💤 Files with no reviewable changes (1)
- internal/client/pagination.go
🚧 Files skipped from review as they are similar to previous changes (10)
- internal/client/client_test.go
- shortcuts/common/runner.go
- internal/client/response_test.go
- internal/client/response.go
- internal/output/envelope_success.go
- internal/client/client.go
- cmd/service/service.go
- cmd/api/api_test.go
- cmd/api/api.go
- cmd/service/service_test.go
Tantanz20020918
force-pushed
the
fix/api-envelope-alignment
branch
from
June 17, 2026 07:03
c8408b7 to
50fd1ee
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1489 +/- ##
==========================================
+ Coverage 73.35% 73.46% +0.11%
==========================================
Files 750 754 +4
Lines 69250 70188 +938
==========================================
+ Hits 50799 51565 +766
- Misses 14711 14816 +105
- Partials 3740 3807 +67 ☔ 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@3944eda0d304c1167dc3e19179e2e294cea478fc🧩 Skill updatenpx skills add Tantanz20020918/cli#fix/api-envelope-alignment -y -g |
Tantanz20020918
force-pushed
the
fix/api-envelope-alignment
branch
3 times, most recently
from
June 17, 2026 09:26
4ac9e0f to
2c3331e
Compare
Tantanz20020918
force-pushed
the
fix/api-envelope-alignment
branch
from
June 17, 2026 09:31
2c3331e to
3944eda
Compare
PR Quality SummaryThe semantic review system could not produce a fully trusted result. This is not reported as a code defect. System status
|
liangshuo-1
approved these changes
Jun 17, 2026
Merged
This was referenced Jul 9, 2026
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
3 participants
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
Align raw API and metadata API success JSON responses with the shortcut envelope contract. This makes successful JSON output consistently expose
ok,identity, and businessdatawithout leaking Lark transport-levelcodeormsgat the top level.Changes
output.WriteSuccessEnvelopeandoutput.SuccessEnvelopeDataininternal/output/envelope_success.go.internal/client/response.goand paginated raw API output incmd/api/api.go.cmd/service/service.goand remove the old raw-response jq pagination helper frominternal/client/pagination.go.RuntimeContext.StreamPagessource-compatible while adapting to the callback error path inshortcuts/common/runner.go.Test Plan
Local verification:
git diff --checkgofmt -l cmd/api/api.go cmd/api/api_test.go cmd/service/service.go cmd/service/service_test.go internal/client/client.go internal/client/client_test.go internal/client/pagination.go internal/client/response.go internal/client/response_test.go shortcuts/common/runner.go internal/output/envelope_success.go internal/output/envelope_success_test.gogo test ./internal/output ./internal/client ./cmd/api ./cmd/service ./shortcuts/common -count=1make unit-testgo vet ./...go mod tidycompleted with nogo.modorgo.sumchangesgo run github.com/golangci/golangci-lint/v2/cmd/golangci-lint@v2.1.6 run --new-from-rev=origin/mainManual PR-preview smoke using pkg.pr.new package:
pkg.pr.newinto an isolated npm prefix and invoked/private/tmp/lark-cli-pr-50fd1ee/bin/lark-clidirectly, without uninstalling or using the machine-globallark-cli.LARKSUITE_CLI_CONFIG_DIR=/private/tmp/lark-cli-pr-sandbox-50fd1ee, initialized the test app, and completed user device authorization with the test account; no existing~/.lark-clilogin state was used.ignore previous instructionsmarker row, and verified shortcutbase +record-list --format jsonreturnsok,identity, anddatawithout top-levelcodeormsg.codeormsg.--page-all --page-size 1andLARKSUITE_CLI_CONTENT_SAFETY_MODE=warnattaches top-level_content_safety_alertwhile preserving originaldata.--jq .data.items[].fields.Notein warn mode returns filtered stdout and still emits the content-safety warning on stderr.6, writes no stdout, and returns a structuredpolicy/content_safetyerror envelope on stderr.calendar calendars listreturnsok,identity, anddatawithout top-levelcodeormsg.description; warn mode attaches_content_safety_alert, and block mode returns empty stdout plus a structuredpolicy/content_safetyerror envelope.ok=false,identity=user,error=...envelopes.Related Issues
N/A
Summary by CodeRabbit
Release Notes
New Features
Improvements
--page-allto run safety checks per streamed page (ndjson/table/csv) and to aggregate results consistently for JSON/jq flows.Tests