refactor: add output emitter contract and differential harness#1899
refactor: add output emitter contract and differential harness#1899sang-neo03 wants to merge 5 commits into
Conversation
github-actions
Bot
added
the
size/L
|
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:
📝 WalkthroughWalkthroughIntroduces ChangesOutput emission
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1899 +/- ##
==========================================
+ Coverage 75.02% 75.10% +0.07%
==========================================
Files 894 895 +1
Lines 94294 94404 +110
==========================================
+ Hits 70746 70899 +153
+ Misses 18137 18095 -42
+ Partials 5411 5410 -1 ☔ 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@b35e5998e403b2ea214713351b6d6724f800f9da🧩 Skill updatenpx skills add larksuite/cli#feat/output-emitter-convergence -y -g |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
internal/output/emitter.go (2)
203-222: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate "unknown format → warn & fall back to json" logic; also silently coerces an unsupported option.
The block at Lines 212-215 is identical to
StreamPage's block at Lines 130-133 — worth extracting into a shared helper (e.g.,resolveFormat(errOut io.Writer, raw string) Format) to avoid drift between the two paths.Separately, per the coding guideline for
**/*.go("never silently fall back, coerce unsupported values... Return a typed validation error when a requested option cannot be honored"), silently rewriting an unrecognizedFormatto JSON (rather than a typed validation error) is exactly the pattern the guideline calls out. This appears intentional here for byte-identical legacy parity (per the PR's stated goal) and the emitter isn't wired to any production caller yet, so I'm not blocking on it — but this should be revisited (return a typed error instead of silently falling back) before this path becomes the production code 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 `@internal/output/emitter.go` around lines 203 - 222, Extract the duplicated unknown-format warning and JSON fallback used by emitFormatted and StreamPage into a shared resolveFormat helper, preserving both paths’ current behavior and warning text. Leave the unsupported-format fallback unchanged for now, but keep the helper boundary suitable for later replacement with a typed validation error.Source: Coding guidelines
224-241: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNew loose-map type + in-place mutation of caller-supplied data.
emitterDataMapis a fresh loose-map type introduced solely to dodgePrintJson's type switch, andm["_notice"] = noticemutates the caller's own map in place — any code retaining a reference todataafter this call will see an injected_noticekey it never added. As per coding guidelines,**/*.gofiles should "Parsemap[string]interface{}into typed structs at boundaries... Do not introduce new loose-map code."This appears to replicate pre-existing
FormatValueJSON-branch behavior for byte-parity, so I'm not asking for behavior change now — but consider copying into a local map (maps.Cloneor manual copy) before injecting_notice, so the emitter never mutates caller-owned data as a side effect.🤖 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/emitter.go` around lines 224 - 241, Update Emitter.printLegacyDataJSON to avoid mutating the caller-supplied map when adding _notice: clone the map into a local copy before injection, while preserving the existing JSON output and PrintJson behavior. Remove the need for the emitterDataMap loose-map workaround if the copied data can be passed through the established typed path.Source: Coding guidelines
🤖 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/emitter_legacy_diff_test.go`:
- Around line 434-465: The TestEmitterStreamPageMatchesPaginationLegacyOracle
cases must cover the opts.JQ != "" branch. Add a jq oracle case that supplies a
non-empty JQ expression, then validate the resulting error with errors.As and
assert the ValidationError.Param field directly; do not rely solely on
assertEquivalentError, which only compares ProblemOf.
In `@internal/output/emitter.go`:
- Around line 174-180: Update the Raw no-JQ branch in the emitter’s Success
method to return the error from enc.Encode(env) instead of discarding it, so
output write failures propagate to callers. Add coverage in
TestEmitterPropagatesOutputError for Raw: true with an empty JQ value and a
failing writer.
---
Nitpick comments:
In `@internal/output/emitter.go`:
- Around line 203-222: Extract the duplicated unknown-format warning and JSON
fallback used by emitFormatted and StreamPage into a shared resolveFormat
helper, preserving both paths’ current behavior and warning text. Leave the
unsupported-format fallback unchanged for now, but keep the helper boundary
suitable for later replacement with a typed validation error.
- Around line 224-241: Update Emitter.printLegacyDataJSON to avoid mutating the
caller-supplied map when adding _notice: clone the map into a local copy before
injection, while preserving the existing JSON output and PrintJson behavior.
Remove the need for the emitterDataMap loose-map workaround if the copied data
can be passed through the established typed path.
🪄 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: 9964bff4-3fb6-48e0-b7c2-2ebece4a3bba
📒 Files selected for processing (2)
internal/output/emitter.gointernal/output/emitter_legacy_diff_test.go
Introduce a leaf Emitter in internal/output that composes the existing output primitives (content-safety scan, envelope, jq, format rendering, notice) behind a single command-scoped port. The emitter is unwired: no production caller is migrated, so CLI output stays byte-for-byte unchanged. A differential test harness drives the real legacy entry points (RuntimeContext.Out/OutRaw/OutFormat/..., WriteSuccessEnvelope and the pagination formatter) and asserts byte-identical stdout/stderr plus typed errors, locking behavior before later slices migrate callers.
84d1f97 to
001e2f8
Compare
|
Both CodeRabbit findings are addressed in 001e2f8:
The only remaining red check is @coderabbitai review |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
fd emitter.go internal/output --exec sed -n '1,260p' {}Length of output: 7680 Confirmed both fixes in the current
Both look correctly implemented and match the description. Kicking off a full review now to check the rest of the diff, including the new test file. ✅ Action performedReview finished.
|
- split Emitter.Success/PartialFailure and drop EmitOptions.OK so a missing ok flag can no longer silently emit ok:false - give StreamPage its own StreamOptions (format + pretty) instead of reusing EmitOptions, making "jq needs aggregation" a compile-time fact - pin the Emitter jq-error contract (returns error, writes no stderr); the caller adapter re-emits the legacy stderr line on migration - add in-package tests driving the real apiPaginate/servicePaginate over a mock transport: multi-page aggregation, empty-result fallback, MarkRaw handling, and the business-error raw-response red line
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 `@cmd/api/api_paginate_test.go`:
- Around line 36-52: Update the test-client helpers in
cmd/api/api_paginate_test.go lines 36-52 and
cmd/service/service_paginate_test.go lines 36-52 to use cmdutil.TestFactory(t,
config) instead of constructing the client directly, and set
LARKSUITE_CLI_CONFIG_DIR to t.TempDir() in each suite to isolate configuration
state.
🪄 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: ca931a8c-643b-4d7c-ab6a-4e016c330a6e
📒 Files selected for processing (4)
cmd/api/api_paginate_test.gocmd/service/service_paginate_test.gointernal/output/emitter.gointernal/output/emitter_legacy_diff_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/output/emitter.go
| func newAPIPaginateTestClient(t *testing.T, rt http.RoundTripper) *client.APIClient { | ||
| t.Helper() | ||
| sdk := lark.NewClient("test-app", "test-secret", | ||
| lark.WithEnableTokenCache(false), | ||
| lark.WithLogLevel(larkcore.LogLevelError), | ||
| lark.WithHttpClient(&http.Client{Transport: rt}), | ||
| ) | ||
| return &client.APIClient{ | ||
| SDK: sdk, | ||
| ErrOut: &bytes.Buffer{}, | ||
| Credential: credential.NewCredentialProvider(nil, nil, &apiPaginateTokenResolver{}, nil), | ||
| Config: &core.CliConfig{ | ||
| AppID: "test-app", | ||
| AppSecret: "test-secret", | ||
| Brand: core.BrandFeishu, | ||
| }, | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Use the standard HTTP test harness in both pagination suites.
Both helpers bypass required factory wiring and configuration isolation.
cmd/api/api_paginate_test.go#L36-L52: usecmdutil.TestFactory(t, config)and setLARKSUITE_CLI_CONFIG_DIRtot.TempDir().cmd/service/service_paginate_test.go#L36-L52: apply the same factory and configuration-isolation setup.
As per coding guidelines, HTTP-mocked tests must use cmdutil.TestFactory(t, config) and isolate the configuration directory.
📍 Affects 2 files
cmd/api/api_paginate_test.go#L36-L52(this comment)cmd/service/service_paginate_test.go#L36-L52
🤖 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_paginate_test.go` around lines 36 - 52, Update the test-client
helpers in cmd/api/api_paginate_test.go lines 36-52 and
cmd/service/service_paginate_test.go lines 36-52 to use cmdutil.TestFactory(t,
config) instead of constructing the client directly, and set
LARKSUITE_CLI_CONFIG_DIR to t.TempDir() in each suite to isolate configuration
state.
Source: Coding guidelines
Replace the hand-rolled RoundTripper + APIClient construction in the apiPaginate/servicePaginate tests with cmdutil.TestFactory and its httpmock.Registry, and isolate LARKSUITE_CLI_CONFIG_DIR to t.TempDir(), matching the repo's standard HTTP-mocked test convention. Assertions and coverage (multi-page aggregation, empty-result fallback, MarkRaw, and the business-error raw-response red line) are unchanged.
Migrate the success-output surfaces onto internal/output's Emitter, byte-for-byte identical (proven by frozen golden diffs and the real paginate/HandleResponse tests): - RuntimeContext.Out/OutRaw/OutFormat/OutFormatRaw/OutPartialFailure now build an Emitter and call Success/PartialFailure; emit and outFormat are removed. An adapter maps the returned error back to the legacy outputErrOnce / jq-error stderr / exit-code behavior. - WriteSuccessEnvelope degrades to a thin Emitter.Success delegate; its 8 callers are unchanged. - apiPaginate/servicePaginate stream pages via Emitter.StreamPage; the aggregate and business-error raw-response branches are untouched. - HandleResponse routes its non-JSON structured-response branch through Emitter.Success. Frozen golden fixtures replace the runtime legacy oracles so the differential harness cannot go self-referential after migration.
github-actions
Bot
added
size/XL
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/common/runner.go`:
- Around line 680-692: Update RuntimeContext.handleEmitterError in
shortcuts/common/runner.go:680-692 to record every non-nil emitter error in
outputErr while only printing the error when JqExpr is active. Ensure OutRaw at
shortcuts/common/runner.go:718-724 preserves the returned writer error in
outputErr, and update shortcuts/common/runner_jq_test.go:124-135 to assert that
the writer error is retained rather than discarded.
🪄 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: 1cf55f0a-1e99-40a0-b999-7dfcfb8031f4
📒 Files selected for processing (13)
cmd/api/api.gocmd/api/api_paginate_test.gocmd/service/service.gocmd/service/service_paginate_test.gointernal/client/response.gointernal/client/response_test.gointernal/output/emitter_legacy_diff_test.gointernal/output/envelope_success.gointernal/output/testdata/runtime_context_legacy.golden.jsoninternal/output/testdata/write_success_envelope_legacy.golden.jsonshortcuts/common/runner.goshortcuts/common/runner_contentsafety_test.goshortcuts/common/runner_jq_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/api/api_paginate_test.go
| func (ctx *RuntimeContext) handleEmitterError(err error) { | ||
| if err == nil { | ||
| return | ||
| } | ||
| var contentSafetyErr *errs.ContentSafetyError | ||
| if errors.As(err, &contentSafetyErr) { | ||
| ctx.outputErrOnce.Do(func() { ctx.outputErr = err }) | ||
| return | ||
| } | ||
| if ctx.JqExpr != "" { | ||
| fmt.Fprintf(ctx.IO().ErrOut, "error: %v\n", err) | ||
| ctx.outputErrOnce.Do(func() { ctx.outputErr = err }) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Preserve raw-output write failures through RuntimeContext.
Emitter.Success now reports raw encoder failures, but handleEmitterError drops them unless jq is active, allowing truncated stdout with a successful exit.
shortcuts/common/runner.go#L680-L692: record every non-nil emitter error; only conditionally print jq errors.shortcuts/common/runner.go#L718-L724: ensureOutRawleaves the returned writer error inoutputErr.shortcuts/common/runner_jq_test.go#L124-L135: invert the test to require the writer error inoutputErr.
As per coding guidelines, output paths must never discard writes. <coding_guidelines>
📍 Affects 2 files
shortcuts/common/runner.go#L680-L692(this comment)shortcuts/common/runner.go#L718-L724shortcuts/common/runner_jq_test.go#L124-L135
🤖 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 `@shortcuts/common/runner.go` around lines 680 - 692, Update
RuntimeContext.handleEmitterError in shortcuts/common/runner.go:680-692 to
record every non-nil emitter error in outputErr while only printing the error
when JqExpr is active. Ensure OutRaw at shortcuts/common/runner.go:718-724
preserves the returned writer error in outputErr, and update
shortcuts/common/runner_jq_test.go:124-135 to assert that the writer error is
retained rather than discarded.
Source: Coding guidelines
1 participant
Summary
The success-output path (content-safety scan → envelope → jq → format render → write) has no single owner: it is assembled at two envelope points (
RuntimeContext.emit,WriteSuccessEnvelope) and re-implemented across several format-routing functions, which have drifted (raw/Meta support,MarkRaw, the jq safety-warning). This PR lands the foundation for converging them — a single leafEmitterplus a byte-level differential harness — and is deliberately unwired: no production caller is migrated, so CLI output stays byte-for-byte unchanged and later slices can migrate callers with machine-proven equivalence.Changes
internal/output/emitter.go: a command-scopedEmitterthat composes the existing leaf primitives (ScanForSafety,Envelope/PrintJson,JqFilter/JqFilterRaw,FormatValue,PaginatedFormatter,WriteAlertWarning) behindSuccessandStreamPage. It captures out/errOut/identity/color/notice at construction, so emission never reads the global notice hook oros.Stdout.EmitOptionsparameterizes the observed differences between the legacy emitters (Raw,OK,Meta,Format,JQ,DryRun,Pretty,JQSafetyWarning);internal/outputstays a leaf (noclient/cmdutildependency).internal/output/emitter_legacy_diff_test.go: a differential harness that drives the real legacy entry points (RuntimeContext.Out/OutRaw/OutPartialFailure/OutFormat/OutFormatRaw,WriteSuccessEnvelope, the pagination formatter) and asserts byte-identical stdout/stderr and typed errors across raw / ok / meta / jq / notice, all formats, content-safety fail-open/fail-closed, and pagination.Test Plan
make unit-testpassedgo build ./.../go vet ./...clean;golangci-lint0 issues;go mod tidyno diffGOTOOLCHAIN=go1.25.4 go test -race ./internal/output)grep NewEmitter(finds only the definition); the six production output files show an empty diff vsmain;go list -deps ./internal/outputexcludesclient/cmdutilRelated Issues
N/A
Summary by CodeRabbit
New Features
Bug Fixes
Tests