refactor: add output emitter contract and differential harness#1899
refactor: add output emitter contract and differential harness#1899sang-neo03 wants to merge 1 commit into
Conversation
github-actions
Bot
added
the
size/L
|
Important Review skippedNo new commits to review since the last review. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughIntroduces ChangesOutput emission
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant Emitter
participant SafetyProvider
participant NoticeProvider
participant OutputWriter
Caller->>Emitter: Success or StreamPage(data, options)
Emitter->>SafetyProvider: Scan(data)
SafetyProvider-->>Emitter: Alert or scan result
Emitter->>NoticeProvider: Notice()
NoticeProvider-->>Emitter: Notice value
Emitter->>OutputWriter: Emit envelope or formatted page
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 74.66% 74.70% +0.03%
==========================================
Files 877 879 +2
Lines 91731 92007 +276
==========================================
+ Hits 68494 68734 +240
- Misses 17926 17947 +21
- Partials 5311 5326 +15 ☔ 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@001e2f80fad31baa607ba18d5d23cccf29700b0a🧩 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.
sang-neo03
force-pushed
the
feat/output-emitter-convergence
branch
from
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.
|
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