refactor(output)!: emitter follow-ups from PR #1899 (strict Format, pretty scan, jq safety, pagination merge)#1998
Open
sang-neo03 wants to merge 20 commits into
Open
refactor(output)!: emitter follow-ups from PR #1899 (strict Format, pretty scan, jq safety, pagination merge)#1998sang-neo03 wants to merge 20 commits into
sang-neo03 wants to merge 20 commits into
Conversation
…gal combos An unknown --format now fails with a typed ValidationError instead of printing a stderr warning and silently degrading to JSON, and unfulfillable combinations are rejected at the flag boundary rather than dropped silently. - Add FormatPretty to the Format enum and ParseFormatStrict, which returns a typed validation error (param --format) for any unrecognized format. - EmitOptions.Format / StreamOptions.Format / Emitter.streamFormat are now the typed output.Format; the upstream .String() -> ParseFormat round-trip and the Emitter's internal unknown-format fallback (printLegacyDataJSON) are removed. - Strict parsing runs at the api, service, and shortcut boundaries, before the dry-run branch so both dry-run and emit reject an unknown format. The strict contract applies only to the framework-injected --format; a shortcut that declares its own format flag (base +record-* markdown|json, mail +watch json|data) keeps its own enum, validated by validateEnumFlags. - The raw api/service commands reject --format pretty on the emit path (no response pretty renderer) while preserving the dry-run plain-text preview; the check runs before confirmation and client init. - ValidateJqFlags classifies the format via ParseFormat so --jq's JSON-only check is case-insensitive and single-sourced: --format JSON --jq no longer mis-rejects, while any non-JSON value (including a shortcut's markdown/data) still conflicts with --jq. BREAKING CHANGE: an unknown --format value (e.g. a typo like `--format tabel`) is now a typed validation error with a non-zero exit code instead of a stderr warning plus JSON output. Scripts that relied on the unknown-format JSON fallback must pass a valid format (json, ndjson, table, csv, or pretty).
The pretty path scanned the structured `data` argument but rendered via an opaque closure, so a renderer that printed content absent from `data` could bypass content-safety in block mode. Render pretty output into a buffer, run the safety scan on the actual rendered text, and only copy to stdout when it passes — the bytes that reach stdout are now exactly what was scanned. Applies to both Success's pretty path and StreamPage's pretty branch; json/table/csv/ ndjson are unchanged (they render from the scanned data directly).
…fety alert When --jq is applied, the jq expression can filter the _content_safety_alert field out of stdout, hiding the warning. Whether a stderr fallback warning was written used to depend on a caller-set EmitOptions.JQSafetyWarning flag, so the raw api/service paths warned but shortcut commands did not — the safety alert was silently lost. Remove the flag and always write the stderr warning when jq is applied and an alert exists.
…Output apiPaginate and servicePaginate were near-identical; merge them into one shared client.PaginateToOutput. The two call-site differences are injected: checkErr (both pass APIClient.CheckResponse) and markErr (cmd/api passes errs.MarkRaw, cmd/service passes nil). markErr wraps only the PaginateAll / StreamPages / checkErr errors — never the WriteSuccessEnvelope return — preserving both commands' exact stdout/stderr bytes and error semantics. Pure refactor.
github-actions
Bot
added
domain/calendar
|
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:
📝 WalkthroughWalkthroughThe CLI now strictly validates output formats, adds typed ChangesFormat contracts and emission
Centralized pagination
Shortcut integration
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant APIServiceCommand
participant PaginateToOutput
participant APIClient
participant Emitter
APIServiceCommand->>PaginateToOutput: provide format and pagination options
PaginateToOutput->>APIClient: request pages
APIClient-->>PaginateToOutput: return page data
PaginateToOutput->>Emitter: emit streamed or aggregated output
Emitter-->>APIServiceCommand: write output or return error
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 |
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@6249392823e14ac5b48bad9f0aae14d5c3c925e6🧩 Skill updatenpx skills add larksuite/cli#refactor/output-emitter-followups -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: 1
🧹 Nitpick comments (1)
cmd/api/api.go (1)
261-268: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the shared pretty-rejection contract into one helper.
cmd/api/api.goandcmd/service/service.goeach hardcode the identicalFormatPrettyrejection (same subtype, message, and param) for their raw emit paths. A single shared helper keeps the two raw commands' "pretty is shortcut-only" contract from silently drifting apart.
cmd/api/api.go#L261-L268: replace the inline check with a call to a shared helper (e.g.output.RejectPretty(format)).cmd/service/service.go#L415-L423: replace the inline check with the same shared helper call.♻️ Proposed shared helper
// in internal/output (e.g. format_type.go) // RejectPretty returns a typed validation error if f is FormatPretty. Raw // api/service commands have no pretty renderer for responses and must reject // it before client init rather than silently falling back. func RejectPretty(f Format) error { if f != FormatPretty { return nil } return errs.NewValidationError(errs.SubtypeInvalidArgument, "--format pretty is not supported here (use json, ndjson, table, or csv)"). WithParam("--format") }- if format == output.FormatPretty { - return errs.NewValidationError(errs.SubtypeInvalidArgument, - "--format pretty is not supported here (use json, ndjson, table, or csv)"). - WithParam("--format") - } + if err := output.RejectPretty(format); err != nil { + return err + }🤖 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.go` around lines 261 - 268, Extract the identical FormatPretty validation into a shared output.RejectPretty helper that returns nil for other formats and preserves the existing typed error, message, and parameter. Replace the inline checks in cmd/api/api.go lines 261-268 and cmd/service/service.go lines 415-423 with calls to this helper.
🤖 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_test.go`:
- Around line 124-184: Add test isolation for the cmd/api tests by setting
LARKSUITE_CLI_CONFIG_DIR to a per-test temporary directory via t.Setenv near the
setup in TestApiCmd_UnknownFormat_Rejected, TestApiCmd_Pretty_RejectedOnEmit,
and TestApiCmd_Pretty_PreservedOnDryRun, or establish equivalent package-wide
isolation with TestMain. Ensure cmdutil.TestFactory cannot access the
developer’s real configuration.
---
Nitpick comments:
In `@cmd/api/api.go`:
- Around line 261-268: Extract the identical FormatPretty validation into a
shared output.RejectPretty helper that returns nil for other formats and
preserves the existing typed error, message, and parameter. Replace the inline
checks in cmd/api/api.go lines 261-268 and cmd/service/service.go lines 415-423
with calls to this helper.
🪄 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: e4f75c0e-b4fe-48d1-ba65-a4c77d9df2f6
📒 Files selected for processing (19)
cmd/api/api.gocmd/api/api_paginate_test.gocmd/api/api_test.gocmd/service/service.gocmd/service/service_paginate_test.gocmd/service/service_test.gointernal/client/paginate_emit.gointernal/client/response.gointernal/output/emitter.gointernal/output/emitter_contract_test.gointernal/output/emitter_legacy_compat_test.gointernal/output/envelope_success.gointernal/output/format_type.gointernal/output/format_type_test.gointernal/output/jq.gointernal/output/jq_test.gointernal/output/testdata/runtime_context_legacy.golden.jsonshortcuts/calendar/calendar_test.goshortcuts/common/runner.go
…ormat Address code-review findings on the emitter follow-ups: - Large pretty output is scanned in overlapping 64 KiB windows instead of one string, so content past the safety scanner's 128 KiB per-string cap is no longer skipped — this was a content-safety bypass reintroduced by the buffer-then-scan change. - Feed the canonical Format.String() into the dry-run path (api/service/ shortcut) so a mixed-case --format Pretty still renders the plain-text preview instead of falling through to the JSON envelope. - The raw api/service unknown-format error lists only json/ndjson/table/csv, not the shortcut-only pretty, so it no longer suggests a value those commands reject.
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 (1)
cmd/service/service_test.go (1)
830-836: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAssert the validation parameter, not only the message.
This test does not verify that the typed error contains
Param == "--format", so removingWithParam("--format")would still pass. Add anerrors.Asassertion for*errs.ValidationError.As per coding guidelines, command error-path tests must assert typed metadata including
param. Based on learnings,errs.ProblemOfexposes noParam; useerrors.Aswith*errs.ValidationErrorfor this field. The production path incmd/service/service.goattaches this parameter.🤖 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_test.go` around lines 830 - 836, Enhance the validation error test around requireProblem to extract the typed error with errors.As into *errs.ValidationError and assert its Param equals "--format". Keep the existing category, subtype, and message assertions, while verifying the production-attached parameter metadata directly.Sources: Coding guidelines, Learnings
🧹 Nitpick comments (1)
shortcuts/common/runner_jq_test.go (1)
355-356: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the standard isolated factory for executable shortcut tests.
Both new tests execute
runShortcutbut construct a manualcmdutil.Factory. Usecmdutil.TestFactory(t, config)and the repository’s required configuration isolation pattern so these tests do not diverge from the standard runtime setup.As per coding guidelines, test factories must use
cmdutil.TestFactory(t, config)and isolateLARKSUITE_CLI_CONFIG_DIR. Based on learnings, direct factory construction is appropriate only for mount-only tests; these tests executerunShortcut.Also applies to: 380-381
🤖 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_jq_test.go` around lines 355 - 356, Replace the manual factory setup in both tests around newTestFactory and newTestShortcutCmd with cmdutil.TestFactory(t, config), applying the repository’s standard LARKSUITE_CLI_CONFIG_DIR isolation pattern. Keep the runShortcut execution unchanged and use direct factory construction only for mount-only tests.Sources: Coding guidelines, Learnings
🤖 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_jq_test.go`:
- Around line 385-392: Update the unknown-format test around runShortcut to
inspect the typed validation problem via errs.ProblemOf and assert
errs.CategoryValidation and errs.SubtypeInvalidArgument, while retaining
errors.As to extract Param from *errs.ValidationError. Preserve the existing
--format and “pretty” message assertions, using ParseFormatStrict’s defined
typed values.
---
Outside diff comments:
In `@cmd/service/service_test.go`:
- Around line 830-836: Enhance the validation error test around requireProblem
to extract the typed error with errors.As into *errs.ValidationError and assert
its Param equals "--format". Keep the existing category, subtype, and message
assertions, while verifying the production-attached parameter metadata directly.
---
Nitpick comments:
In `@shortcuts/common/runner_jq_test.go`:
- Around line 355-356: Replace the manual factory setup in both tests around
newTestFactory and newTestShortcutCmd with cmdutil.TestFactory(t, config),
applying the repository’s standard LARKSUITE_CLI_CONFIG_DIR isolation pattern.
Keep the runShortcut execution unchanged and use direct factory construction
only for mount-only tests.
🪄 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: 6da1fc11-4279-4390-8a71-ea25549c0584
📒 Files selected for processing (8)
cmd/api/api.gocmd/api/api_test.gocmd/service/service.gocmd/service/service_test.gointernal/output/emitter.gointernal/output/emitter_contract_test.goshortcuts/common/runner.goshortcuts/common/runner_jq_test.go
🚧 Files skipped from review as they are similar to previous changes (6)
- shortcuts/common/runner.go
- cmd/service/service.go
- cmd/api/api.go
- cmd/api/api_test.go
- internal/output/emitter_contract_test.go
- internal/output/emitter.go
Comment on lines
+385
to
+392
| err := runShortcut(cmd, f, s, false) | ||
| if err == nil { | ||
| t.Fatal("expected a validation error for unknown --format") | ||
| } | ||
| assertValidationParam(t, err, "--format") | ||
| if !strings.Contains(strings.ToLower(err.Error()), "pretty") { | ||
| t.Fatalf("shortcut unknown-format error = %v, want pretty in allowed choices", err) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the complete typed validation problem.
The visible assertions check the parameter and message text, but do not assert errs.CategoryValidation and errs.SubtypeInvalidArgument. Add those checks through errs.ProblemOf; retain errors.As for Param.
As per coding guidelines, command error-path tests must verify category, subtype, and parameter. Based on learnings, Param must be read from *errs.ValidationError, not errs.ProblemOf. The ParseFormatStrict contract in internal/output/format_type.go defines the expected typed values.
🤖 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_jq_test.go` around lines 385 - 392, Update the
unknown-format test around runShortcut to inspect the typed validation problem
via errs.ProblemOf and assert errs.CategoryValidation and
errs.SubtypeInvalidArgument, while retaining errors.As to extract Param from
*errs.ValidationError. Preserve the existing --format and “pretty” message
assertions, using ParseFormatStrict’s defined typed values.
Sources: Coding guidelines, Learnings
Further review fixes: - Normalize the framework-injected --format to its canonical lowercase and write it back to the runtime context and the cobra flag, so shortcuts that branch on the exact value (e.g. Format == "pretty") behave correctly for mixed-case input like --format Pretty, which previously slipped past those checks and produced empty output. - Size the pretty rendered-text scan window to the content-safety scanner's native 128 KiB per-string capacity (was 64 KiB) so the windowing is no more restrictive than scanning the raw value; keep the 4 KiB overlap for matches crossing a window boundary.
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_jq_test.go`:
- Around line 385-386: Update the execution-path test around newTestShortcutCmd
to use cmdutil.TestFactory(t, config) instead of newTestFactory(). Isolate
LARKSUITE_CLI_CONFIG_DIR with t.Setenv and t.TempDir() before creating the
command, while preserving the existing shortcut execution assertions.
🪄 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: 3530ff2f-fe48-4e13-8d0c-0258fbbb2a3b
📒 Files selected for processing (4)
internal/output/emitter.gointernal/output/emitter_contract_test.goshortcuts/common/runner.goshortcuts/common/runner_jq_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- shortcuts/common/runner.go
- internal/output/emitter_contract_test.go
- internal/output/emitter.go
Address the owner review's blocking finding on PR #1998: the 128 KiB windowed pretty scan could be bypassed — a match straddling a window boundary (via the default instruction_override rule's unbounded \s+) matched the full text but neither window, so block mode still wrote it to stdout. - Scan the complete rendered text as one string (no windows): correct regex semantics, and the []any window round-trip through normalize disappears. - Add a no-truncate full-text scan path (extcs.ScanRequest.FullText, additive) so content past the scanner's 128 KiB per-string cap is still scanned; latency stays bounded by the existing 100 ms scan timeout. The structured API-response scan path keeps its cap. - Block mode now FAILS CLOSED when a scan cannot complete (timeout/error/panic): nothing is written and a typed ContentSafetyError is returned. warn/off keep failing open. This intentionally changes the §0.3 content-safety red line for block mode; the legacy oracle golden is updated accordingly. - Report an unknown --format before the --jq conflict (with the --format param), and validate the framework --format on the --print-schema path too. Regression tests: cross-window instruction_override payload is now blocked; full-text scan catches matches beyond the per-string cap; regex boundary semantics preserved; block-mode fail-closed on scan timeout/error.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1998 +/- ##
=======================================
Coverage ? 75.10%
=======================================
Files ? 906
Lines ? 96394
Branches ? 0
=======================================
Hits ? 72399
Misses ? 18422
Partials ? 5573 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
… format Address Codex review low-priority items: - extension/contentsafety ScanRequest.FullText now documents that providers must scan the full string with no per-string truncation, and the Provider interface comment requires implementations to honor it. - RuntimeContext.OutFormat/OutFormatRaw check ParseFormat's ok and return a typed internal error for an unsupported format instead of silently degrading to JSON; drop the stale 'validated by ParseFormatStrict' comments (that gate only covers framework-injected formats, not self-declared ones).
…erer Address the owner review high-priority items on PR #1998: - H1: a content-safety scan aborted by context cancellation returned (nil, nil), indistinguishable from a clean scan; when the result channel raced ctx.Done() the select could pick it and block mode would fail OPEN. scanner.walk now returns ctx.Err() so an aborted scan surfaces as an error (block fails closed via the existing path), and runContentSafety re-checks ctx.Err() after receiving a result as a second layer. Regression tests: a provider that returns (nil,nil) only after ctx.Done() is blocked in block mode; walk/scan cancellation surface errors. - H2: emitPretty no longer silently emits JSON when no pretty renderer is supplied — it returns a typed validation error (param --format). So --format pretty on a shortcut without a renderer now errors instead of returning JSON (intentional output-contract tightening; the legacy oracle golden's pretty_without_renderer case is updated to the error shape). - Add a CLI dry-run E2E for the format contract (mixed-case pretty preview, unknown format rejected pre-request, error carries category/subtype/param). - runner jq tests use cmdutil.TestFactory with an isolated config dir.
…retty fallback Second-round owner review follow-ups on PR #1998. - P1: --format pretty on a command that has no pretty renderer no longer errors after the work already ran. For a write that path mutated remote state, then exited non-zero, so automation treated it as a failure and retried, creating duplicate resources. The emitter now writes a stderr warning and falls back to the JSON envelope (exit 0); StreamPage falls back to NDJSON with a single warning. Reads and writes behave identically. - P1: make full-text content-safety scanning a DETECTABLE capability. Add extension/contentsafety.FullTextProvider; block mode requires it and returns scan-incomplete (blocked, empty stdout) for any provider that cannot guarantee a complete scan. A legacy provider that silently truncates can no longer let a match past the truncation point reach stdout. - P1: structured output (json/table/csv/ndjson) is scanned in full under block mode. Per-string 128 KiB truncation and depth-cap stops now surface as scan-incomplete and block, instead of emitting data that was only partially scanned. - P2: invalid Format enum values error instead of silently degrading. Format.Valid(), String() renders unknown(N) for out-of-range values, and Success / StreamPage / WriteFormatted / PaginatedFormatter.WritePage return a typed internal error rather than defaulting to JSON or writing nothing. - P3: the content-safety scan-context factory is an injected dependency rather than a mutable package global, so parallel tests cannot interfere.
…e format Address the pr-review on PR #1998. - P1 (security): the content-safety scanner walked map VALUES only, so a rule match hiding in a map KEY (which json/ndjson/table/csv all emit) slipped past block mode — a deterministic structured-output bypass. walk now scans each key before recursing into its value. - P1 (CI): remove the now-unreachable exported ScanRenderedText. The scan path moved onto Emitter.scanForSafety, leaving ScanRenderedText dead, which failed the required `deadcode` check. ScanForSafety is still used and stays. - P2: PartialFailure now validates opts.Format like Success/StreamPage and returns a typed internal error for an invalid enum instead of silently emitting JSON; added to the invalid-format test table. - P2: the streaming pretty fallback warning said "showing JSON" but the output is NDJSON; corrected the message (and its test) to "showing NDJSON instead". Tests: TestWalk_ScansMapKeys and TestProvider_ScanDetectsInjectionInMapKey (Scan + ScanFullText) cover the map-key scan; the invalid-format table now covers PartialFailure.
…ield joins The content-safety scan ran over the structured data, but table joins cells with whitespace and jq can concatenate fields, so a rule match can form only in the rendered output — the scan never saw it and block mode still wrote it to stdout (e.g. cells "ignore" + "previous instructions" render as "ignore previous instructions", matching instruction_override). Scan the exact bytes that will be written instead: - table, csv, ndjson, pretty, and streamed pages now render into a buffer, scan that buffer as full text, and copy to stdout only if the scan does not block (a warn-mode alert still goes to stderr). - the jq path scans the rendered jq output before writing, catching expressions like '.data.a + " " + .data.b' that concatenate fields. - the JSON envelope keeps scanning its data: JSON serialization keeps per-field punctuation between values, so a whitespace-joined cross-field match cannot form there, and this preserves the embedded content-safety alert without a second scan. - removed the now-unused emit() helper. Tests: TestEmitterBlockScansRenderedCrossFieldConcatenation covers table cross-column values, table cross-column keys, a scanned csv render, and a jq concatenation (all assert ContentSafetyError + empty stdout in block mode). The api/service streaming content-safety tests now assert the scan sees the rendered page bytes.
… wire remaining commands Complete the content-safety and output-contract follow-ups across the emitter, pagination, and the commands that emit directly. - Content safety scans the exact rendered bytes for table/csv/ndjson/pretty/jq and per streamed page, so a match formed only in the rendered output (joined table cells, a jq concatenation) cannot slip past block mode. The JSON envelope keeps its data scan (JSON punctuation prevents whitespace-joined cross-field matches) and still embeds the alert. - warn pagination writes each page as it arrives; block pagination buffers and commits atomically with a 64 MiB cap, failing closed past the limit. - --format pretty without a dedicated renderer falls back to human-readable output rather than JSON; OutPartialFailure keeps the caller's table/csv/ ndjson/pretty choice while json/jq keep the ok:false envelope. - auth scopes --format pretty writes business data to stdout and propagates write errors through the emitter. - event +subscribe exposes only json/ndjson (--json kept as an alias, conflict checked before run); event/mail-triage/mail-watch/record-markdown/schema output now run the unified content-safety scan; mail watch no longer logs addresses or sender info. - calendar/minutes/vc pagination progress moved to stderr so csv/ndjson stdout stays parseable; non-list paginated responses keep the requested format. - wrap the flag-parse error with %w in localfileio path handling.
github-actions
Bot
added
domain/mail
The prior change dropped the mailbox from the subscribe/filter progress lines and the sender from the prompt-injection security warning. That logging content is the command's own operational/security surface, not the output framework's concern, so restore it. The unified emitter routing is kept.
PaginateToOutput had 11 positional parameters (writers, callbacks, pagination knobs), which read poorly at the call sites and invited positional-argument mistakes. Introduce PaginateOutputOptions and reduce the signature to (ctx, opts). The two production call sites (cmd/api, cmd/service) now pass named fields; the pagination tests use a small adapter so each case stays one statement. No behavior change.
Real binary-vs-main comparison caught a regression: `mail +triage`'s default
view (table) rendered as a JSON array. The cause is the emitter routing —
EmitValue(rows, "table") on []map[string]interface{} goes through
emitValue→WriteTable directly, which (unlike WriteFormatted) does not
ExtractItems-normalize the shape, so it fell back to JSON. Restore
output.PrintTable / output.PrintJson for the table and json branches so the
rendered output matches the prior behavior. The filter-schema keeps OutJSON: it
must emit JSON regardless of the command's --format (verified by the
Format="data" test).
Its callers were routed through the emitter, leaving PrintNdjson unreachable, which the incremental deadcode CI gate rejects. WriteNDJSON remains for the emitter path. No behavior change.
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
Follow-ups from the PR #1899 (output Emitter convergence) owner review, each establishing one verifiable invariant. Scope kept to items ①, ②, ④ and the pagination-merge half of ⑤; items ③ (poisoned-stream state machine) and the notice-ownership half of ④ are deliberately deferred.
Changes
refactor(output)!— strict typed Format (--format(e.g. a typo like--format tabel) is now a typedValidationErrorwith a non-zero exit code instead of a stderr warning + JSON fallback.FormatgainsFormatPretty;ParseFormatStrictrejects unknown values at the api/service/shortcut boundaries (before the dry-run branch);EmitOptions.Format/StreamOptions.Formatare the typedoutput.Format(no more.String()round-trip); the rawapi/servicecommands reject--format prettyon the emit path while keeping the dry-run preview;ValidateJqFlagsclassifies the format case-insensitively so--format JSON --jqno longer mis-rejects. Shortcuts that declare their own format flag (markdown/data) keep their enum.refactor(output)— scan pretty-rendered output. The pretty path scanned the structureddataargument but rendered via an opaque closure, so a renderer printing content absent fromdatacould bypass content-safety in block mode. Pretty output is now rendered into a buffer, the safety scan runs on the actual rendered text, and stdout receives exactly what was scanned.refactor(output)— always warn on stderr when jq may drop a content-safety alert. Whether the stderr fallback warning was written used to depend on a caller-setJQSafetyWarningflag, soapi/servicewarned but shortcut commands silently lost the alert when--jqfiltered it out. The flag is removed; the Emitter always warns when jq is applied and an alert exists.refactor(output)— unify api/service pagination intoclient.PaginateToOutput.apiPaginateandservicePaginatewere near-identical; merged into one shared function, withcheckErrandmarkErrinjected. Pure refactor, byte-identical output and error semantics.Test Plan
go test -count=1 ./internal/output/ ./cmd/api/ ./cmd/service/ ./shortcuts/common/ ./shortcuts/base/ ./shortcuts/mail/ ./shortcuts/calendar/ ./shortcuts/contact/ ./internal/client/all green.go test -count=1 ./...green except thetests/cli_e2e/*packages, which fail identically on cleanmainfor environmental reasons (require a built binary / live env), unrelated to this change.lark-cli <domain> <command>flow works as expectedRelated Issues
Summary by CodeRabbit
prettyoutput format support, including plain-text dry-run previews.--formatnow strictly validates and fails fast with typed errors (no silent JSON fallback);--format prettyis rejected for normal emit but allowed in dry-run.--jq/--formatcompatibility and consistent JQ safety alert warning behavior.prettybehavior.