refactor: converge success output through a single Emitter that owns the write#1899
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.03% +0.01%
==========================================
Files 894 901 +7
Lines 94294 95321 +1027
==========================================
+ Hits 70746 71528 +782
- Misses 18137 18300 +163
- Partials 5411 5493 +82 ☔ 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@11d456096041344c1951c9f1d08ecdcfe7ebb8ea🧩 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
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
…back printLegacyDataJSON now normalizes via toGeneric first (matching FormatValue), so a struct / named-map payload retains its injected _notice on the unknown-format -> JSON fallback rather than dropping it silently. Add a regression test that fails against the pre-fix path.
Route every Emitter stdout path through a render-to-buffer-then-copy helper so a marshal/render failure leaves stdout empty and surfaces a typed internal error (with cause), and a stdout write failure is propagated instead of silently swallowed. Leaf writers gain error-returning Write* cores; the legacy Print*/FormatValue wrappers keep their exact behavior for unmigrated callers. - handleEmitterError now captures every error, not only the jq/safety branches; flip OutRaw's write-error test to assert propagation. - Clone the map before injecting _notice so a caller's payload is never mutated and an existing _notice is never overwritten. - Preserve jq's own typed error (validation/api) on a bad expression or runtime failure; only wrap genuine stdout write failures. - Split tests: normative emitter_contract_test.go vs frozen emitter_legacy_compat_test.go (base SHA recorded, self-update env vars removed).
sang-neo03
changed the title
- Move the base-SHA note below the copyright header in the renamed legacy-compat test so the license-header check sees a valid header at the top. - Route the leaf wrappers' marshal/format stderr messages through a single legacyStderrf helper (one //nolint:forbidigo) instead of bare os.Stderr, preserving exact legacy behavior for unmigrated direct callers while passing forbidigo; drop the now-unused os imports.
Align FormatAsCSV/FormatAsCSVPaginated and FormatValue/FormatPage's CSV branch with the other leaf wrappers: report only marshal failures, swallow write failures. Previously they emitted a 'csv write error' for the (empty) line and the JSON-fallback write failures that the pre-refactor code ignored, and mislabeled a JSON write failure as a CSV one. Failure-path only; success output is unchanged (golden double-diff still byte-for-byte).
2 participants
Why
The success-output path (content-safety scan → envelope → jq → format render → write) had no single owner: it was assembled at two envelope points (
RuntimeContext.emit,WriteSuccessEnvelope) and re-implemented across several format-routing functions, which had drifted (raw/Meta support,MarkRaw, the jq safety-warning). Write failures were silently swallowed on most paths, and_noticeinjection mutated the caller's map.What this PR does (now)
Converges all success output onto a single command-scoped
internal/output.Emitterand makes that Emitter own the write — proven byte-for-byte on the success path by a differential golden harness driven from the real legacy entry points.Migrated onto the Emitter (byte-for-byte)
RuntimeContext.Out/OutRaw/OutFormat/OutFormatRaw/OutPartialFailure— signatures unchanged;emit/outFormatdeleted.WriteSuccessEnvelope— thin delegate toEmitter.Success.internal/client.HandleResponse— the non-JSON structured-response presentation branch.Emitter.StreamPage.Write-ownership contract (this PR)
Write*cores (WriteJSON,WriteNDJSON,WriteTable,WriteCSV,WriteAlertWarning,PaginatedFormatter.WritePage); legacyPrint*/FormatValuewrappers keep their exact behavior for unmigrated callers.io.Copys to stdout, so a marshal/render failure leaves stdout empty and returns a typed internal error (with cause viaerrors.Unwrap/errors.Is); a stdout write failure is propagated, not swallowed.handleEmitterErrornow captures every error (not only the jq/safety branches)._noticeis injected into amaps.Cloneof the payload — the caller's map is never mutated and an existing_noticeis never overwritten.Contract table (per format)
""/jsonEnvelope--jqEnvelopeJQSafetyWarning)prettytable/csv/ndjsonBehavior changes
os.Stderronly);_noticeno longer mutates the caller's map; struct/named-map payloads keep_noticeon the unknown-format → JSON fallback; jq's typed error is preserved through the buffer.--formatstill falls back to JSON instead of a typed validation error; the pretty renderer does not yet receive the scanned data; the streaming formatter advances column state before confirming the write (write failures are propagated, but the poisoned-state machine is follow-up); notice still flows via the globalPendingNoticeprovider andJQSafetyWarningis still caller-selected.Oracle provenance
4a56748bfa941ff0ee0bfec92e65acac427732b0.RuntimeContext.Out*vs golden) so it cannot self-reference.Test Plan
go build ./.../go vet ./...clean;gofmtcleanGOTOOLCHAIN=go1.25.4 go test -race -count=1 ./internal/output ./shortcuts/common ./cmd/api ./cmd/serviceerr==nil ⇒ fully written, marshal failure → typed internal error + empty stdout, writer failure preserveserrors.Iscause, Emitter does not mutate the caller map, existing_noticenot overwritten, notice read at most once, jq runtime error preserves its typed errorOutRawpropagates (flipped from the old ignore test),runShortcutnon-zero exit on a failing writer, api/service mid-page write failure stops further pagesFollow-ups (out of scope; separate PRs, one verifiable invariant each)
Format: unknown format → typed--formatvalidation error, reject illegal combos (jq+table,raw+csv). (user-visible; needs changelog)PrettyRenderer(w, data, color)receives the scanned data.PendingNotice; notice only inEnvelope.Notice; drop caller-selectedJQSafetyWarning(always warn).PrintJsonpaths (binary/download metadata, event consume) and the pretty dry-run safety bypass; remove theWriteSuccessEnvelopedelegate once callers are migrated.Reviewer guide
contract/golden (
emitter_contract_test.go,emitter_legacy_compat_test.go) →Emitter(emitter.go) →RuntimeContextadapter +handleEmitterError→ api/service pagination.Related Issues
N/A (follow-ups tracked out-of-band, listed above).