Skip to content

feat: add --toon output format for token-optimized CLI output - #215

Open
akb4q wants to merge 5 commits into
InsForge:mainfrom
akb4q:feat/toon-output-format
Open

feat: add --toon output format for token-optimized CLI output#215
akb4q wants to merge 5 commits into
InsForge:mainfrom
akb4q:feat/toon-output-format

Conversation

@akb4q

@akb4q akb4q commented Jul 30, 2026

Copy link
Copy Markdown

Summary

Add a global --toon flag that outputs results in TOON (Token-Oriented Object Notation), designed for LLM consumption — 6-9x more compact than JSON or human-readable tables.

Performance

Measured on insforge projects list:

Format Size vs JSON
TOON 129 B 7.7× smaller
JSON 989 B 1× (baseline)
Human table 1,162 B 0.85×

Usage

insforge projects list --toon
# [2]{ID,Name,Region,Status,AppKey}:
#   uuid-1,my-project,us-east,active,key-1
#   uuid-2,other-project,eu-west,active,key-2

insforge whoami --json --toon
# id: 3db0ba25-...
# name: Alice
# email: alice@example.com

insforge functions list --toon
# [3]{Slug,Name,Status,"Created At"}:
#   func-1,My Function,active,"2026-07-30, 10:00:00 AM"

Implementation

  • TOON format: toonformat.dev — token-optimized notation with 36+ language implementations
  • Module-level flag set via Commander preAction hook, read by outputTable and outputJson at render time — zero changes to any command handler
  • Spec compliance: toonEscapeValue / toonEscapeKey conform to TOON SPEC §7.1-7.3 (escaping, quoting rules, key encoding)
  • Non-structured suppression: outputSuccess/ outputInfo silently skipped in TOON mode to keep output parseable

Testing

  • npm run build: tsup compiles clean (601 KB ESM)
  • npm test: 649 tests pass (1 pre-existing Cloudflare OAuth failure unrelated)
  • Manual integration: projects list, whoami, functions list, orgs list with --toon and --json --toon

Root commands (create, login, feedback, etc.) produce no TOON output — they emit interactive prompts and status messages, not structured data.


Summary by cubic

Adds a global --toon output format for token‑optimized results, ~6–9× smaller than JSON or tables. Uses @toon-format/toon encode() and suppresses non‑structured messages to keep output parseable.

  • New Features

    • Global --toon via pre‑action hook; outputJson uses encode() and tables render as tabular TOON.
    • Spec‑compliant escaping for headers/values; interactive root commands unchanged.
  • Bug Fixes

    • Treat --toon as implying JSON in getRootOpts to avoid empty output and credential loss.
    • Quote structural characters correctly and suppress outputSuccess/outputInfo in TOON mode; add tests for quoting, nulls, single‑doc output, and empty tables.

Written for commit 0b66a55. Summary will update on new commits.

Review in cubic

Note

Add --toon global CLI flag for token-optimized TOON format output

  • Adds a --toon global flag to the CLI that switches all output to TOON format using the @toon-format/toon package.
  • In TOON mode, outputJson encodes data via encode(), outputTable renders a tabular TOON document with header schema and row values, and outputSuccess/outputInfo are silently suppressed.
  • Updates getRootOpts().json in errors.ts to return true when --toon is active, so error-handling code treats TOON mode like JSON mode.
  • Behavioral Change: success and info messages are fully suppressed when --toon is used, which may hide feedback in interactive workflows.

Macroscope summarized 0b66a55.

Summary by CodeRabbit

  • New Features
    • Added a global --toon option for structured TOON-formatted command output.
    • JSON and table responses now support TOON formatting.
    • TOON mode is applied consistently across command actions and error handling.
  • Bug Fixes
    • Improved consistency between TOON and standard JSON output behavior.
    • Success and informational messages are suppressed in TOON mode to preserve structured output.

Add a new global --toon flag that outputs command results in TOON format
(https://toonformat.dev), a token-optimized notation designed for LLM
consumption — 6-9x more compact than JSON or human-readable tables.

Implementation:
- toonEscapeValue / toonEscapeKey: spec-compliant quoting (§7.1-7.2) and
  key encoding (§7.3)
- outputToon: tabular array format for list commands
- outputJsonToon: tabular for uniform arrays, key-value for single objects
- Module-level flag set via Commander preAction hook
- outputSuccess/outputInfo suppressed in TOON mode (non-structured noise)

Changes:
  src/index.ts       — register --toon global option + preAction hook
  src/lib/output.ts  — add TOON formatters + route outputTable/outputJson

Token savings measured on insforge projects list: TOON 129B vs JSON 989B
(7.7x), vs human table 1162B (9x).

Tested: build (tsup), 649 unit tests (1 pre-existing cloudflare failure),
integration on projects list, whoami, functions list, orgs list.
@agent-zhang-beihai

Copy link
Copy Markdown
Contributor

Thanks for the PR, @akb4q! A quick note on our workflow: we ask contributors to open an issue first, get it assigned, then submit a PR that links it (e.g. "Closes #123"). This PR isn't linked to any issue. It'll still be reviewed, but please open an issue and claim it (comment that you'd like it assigned to you) so the work is tracked.

@agent-zhang-beihai agent-zhang-beihai Bot added the needs-issue PR isn't linked to any issue — open and claim an issue first, then link it label Jul 30, 2026
@coderabbitai

coderabbitai Bot commented Jul 30, 2026

Copy link
Copy Markdown

Review Change Stack

Walkthrough

The CLI adds a global --toon option. Output helpers encode JSON and tables in TOON format, suppress status messages in TOON mode, and retain standard output otherwise. Tests cover escaping, serialization, status suppression, and tables.

Changes

TOON output mode

Layer / File(s) Summary
TOON serialization and output routing
src/lib/output.ts, src/lib/output.test.ts, package.json
Adds TOON encoding for JSON and tables, suppresses success and info messages in TOON mode, preserves standard output, adds the runtime dependency, and tests the formatting behavior.
Global CLI TOON configuration
src/index.ts
Adds the global --toon option and enables TOON mode before actions execute.
Root option propagation
src/lib/errors.ts
Uses the TOON option as a fallback source for the computed JSON setting.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Possibly related issues

  • InsForge/CLI#216 — Directly covers the --toon CLI output feature implemented here.

Possibly related PRs

  • InsForge/CLI#205 — Both changes modify the global CLI preAction hook, but they handle different options and behaviors.

Poem

A rabbit toggles --toon with care,
TOON-shaped messages fill the air.
Tables hop in rows,
Escaped values pose,
And quiet status leaves output bare.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: adding the global --toon option for token-optimized CLI output.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

Comment thread src/lib/output.ts Outdated
Comment thread src/lib/output.ts Outdated
@greptile-apps

greptile-apps Bot commented Jul 30, 2026

Copy link
Copy Markdown

Greptile Summary

Adds a global TOON output mode.

  • Registers --toon and enables it before command actions.
  • Encodes shared JSON output and tables as TOON.
  • Treats TOON as structured output when commands choose their rendering branch.
  • Suppresses informational and success messages in TOON mode.
  • Adds the TOON encoder dependency and output-format tests.

Confidence Score: 4/5

The PR is not yet safe to merge because login --toon can emit JSON instead of the requested TOON format.

The global hook enables TOON for login and structured branching recognizes the flag, but login serializes results directly with JSON.stringify and bypasses the TOON-aware output helper.

Files Needing Attention: src/index.ts and src/commands/login.ts

Important Files Changed

Filename Overview
src/index.ts Registers global TOON mode, but applying it to every action exposes login paths that bypass the TOON-aware output helper.
src/lib/errors.ts Treats --toon as structured output so command handlers generally select their existing JSON-data branches.
src/lib/output.ts Adds TOON encoding for shared JSON and table output and suppresses unstructured status messages.
src/lib/output.test.ts Covers scalar escaping, nested JSON encoding, message suppression, and tabular TOON output.
package.json Adds the TOON encoder runtime dependency.
package-lock.json Locks the TOON encoder dependency to version 4.1.0 with integrity metadata.

Comments Outside Diff (1)

  1. src/index.ts, line 165-169 (link)

    P1 Login bypasses TOON encoding

    When a non-interactive login flow runs with --toon, the global hook enables TOON mode and the command selects structured output, but the login handlers serialize their results directly with JSON.stringify instead of outputJson, causing consumers expecting TOON to receive JSON.

Reviews (5): Last reviewed commit: "Merge branch 'main' into feat/toon-outpu..." | Re-trigger Greptile

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 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 `@src/lib/output.ts`:
- Around line 17-32: Resolve the ESLint violations in the quote-formatting logic
around needsQuoting: replace the escaped opening bracket in the punctuation
regex with a literal bracket, and remove both control-character regexes in favor
of equivalent character-code checks and replacement handling. Preserve the
existing quoting and escaping behavior, including Unicode escapes for
unsupported control characters.
- Around line 99-124: Replace the ad hoc serialization in outputJsonToon with a
conformant recursive TOON encoder, preferably using the official
`@toon-format/toon` encode API. Preserve heterogeneous array values and nulls,
recursively encode nested objects and arrays rather than stringifying them, and
retain null fields/entries; add decode round-trip coverage for these cases.
🪄 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: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 17e2f641-7d66-42fc-a61f-608a2ac82f7b

📥 Commits

Reviewing files that changed from the base of the PR and between 4353cc6 and ee4a99c.

📒 Files selected for processing (2)
  • src/index.ts
  • src/lib/output.ts

Comment thread src/lib/output.ts Outdated
Comment on lines +17 to +32
const needsQuoting =
s === '' ||
s === 'true' || s === 'false' || s === 'null' ||
/^[+-]?[0-9]+(\.[0-9]+)?([eE][+-]?[0-9]+)?$/.test(s) ||
/^[ \t]|[ \t]$/.test(s) ||
/^[-#]/.test(s) ||
/[:,"\\{}\[\]]/.test(s) ||
/[\x00-\x1f]/.test(s);
if (!needsQuoting) return s;
return '"' + s
.replace(/\\/g, '\\\\')
.replace(/"/g, '\\"')
.replace(/\n/g, '\\n')
.replace(/\r/g, '\\r')
.replace(/\t/g, '\\t')
.replace(/[\x00-\x08\x0b\x0c\x0e-\x1f]/g, c => '\\u' + c.charCodeAt(0).toString(16).padStart(4, '0')) + '"';

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win

Resolve the new ESLint errors.

The escaped [ and both control-character regexes are reported as ESLint errors, leaving this changed file failing lint. Replace the control-character regex checks/replacement with character-code handling and use a literal [.

🧰 Tools
🪛 ESLint

[error] 23-23: Unnecessary escape character: [.

(no-useless-escape)


[error] 24-24: Unexpected control character(s) in regular expression: \x00, \x1f.

(no-control-regex)


[error] 32-32: Unexpected control character(s) in regular expression: \x00, \x08, \x0b, \x0c, \x0e, \x1f.

(no-control-regex)

🤖 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 `@src/lib/output.ts` around lines 17 - 32, Resolve the ESLint violations in the
quote-formatting logic around needsQuoting: replace the escaped opening bracket
in the punctuation regex with a literal bracket, and remove both
control-character regexes in favor of equivalent character-code checks and
replacement handling. Preserve the existing quoting and escaping behavior,
including Unicode escapes for unsupported control characters.

Source: Linters/SAST tools

Comment thread src/lib/output.ts Outdated

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1 issue found across 2 files

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="src/index.ts">

<violation number="1" location="src/index.ts:119">
P1: Using `--toon` alone can produce an empty, human-readable, or raw response instead of TOON. The new flag is parsed by the preAction hook, but command handlers still branch only on `getRootOpts(cmd).json`: `whoami --toon` takes its `outputInfo` branch and both messages are then suppressed, while commands such as `functions invoke` and `metadata` write directly with `console.log` and bypass the TOON renderer. This makes the advertised global format inconsistent unless callers already know to add `--json`; treating TOON as an effective structured-output mode (or explicitly rejecting/documenting it without `--json`) would avoid these silent/mixed responses.</violation>
</file>

Reply with feedback, questions, or to request a fix.

Re-trigger cubic

Comment thread src/lib/output.ts Outdated
Comment thread src/lib/output.ts Outdated
Comment thread src/index.ts
// Global options
program
.option('--json', 'Output in JSON format')
.option('--toon', 'Output in TOON format (token-optimized notation, see toonformat.dev)')

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1: Using --toon alone can produce an empty, human-readable, or raw response instead of TOON. The new flag is parsed by the preAction hook, but command handlers still branch only on getRootOpts(cmd).json: whoami --toon takes its outputInfo branch and both messages are then suppressed, while commands such as functions invoke and metadata write directly with console.log and bypass the TOON renderer. This makes the advertised global format inconsistent unless callers already know to add --json; treating TOON as an effective structured-output mode (or explicitly rejecting/documenting it without --json) would avoid these silent/mixed responses.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/index.ts, line 119:

<comment>Using `--toon` alone can produce an empty, human-readable, or raw response instead of TOON. The new flag is parsed by the preAction hook, but command handlers still branch only on `getRootOpts(cmd).json`: `whoami --toon` takes its `outputInfo` branch and both messages are then suppressed, while commands such as `functions invoke` and `metadata` write directly with `console.log` and bypass the TOON renderer. This makes the advertised global format inconsistent unless callers already know to add `--json`; treating TOON as an effective structured-output mode (or explicitly rejecting/documenting it without `--json`) would avoid these silent/mixed responses.</comment>

<file context>
@@ -116,6 +116,7 @@ program
 // Global options
 program
   .option('--json', 'Output in JSON format')
+  .option('--toon', 'Output in TOON format (token-optimized notation, see toonformat.dev)')
   .option('--forger', 'Play the Forger animation (root command only) and return to the interactive menu')
   .option('--api-url <url>', 'Override Platform API URL')
</file context>

Comment thread src/lib/output.ts Outdated
Comment thread src/lib/output.ts Outdated
Comment thread src/lib/output.ts Outdated
Comment thread src/lib/output.ts Outdated
@akb4q

akb4q commented Jul 30, 2026

Copy link
Copy Markdown
Author

Closes #216

@jwfing jwfing left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review: feat: add --toon output format

Summary: Clean, well-scoped feature (module-level flag routed through outputTable/outputJson, zero handler changes), but a broken regex in the value-quoting logic means values containing the delimiter or other structural characters are emitted unquoted, producing malformed TOON — including on the PR's own headline example.

Requirements context

No matching spec/plan found — this repo keeps specs under docs/specs/ (not docs/superpowers/), and none of the three present (diagnose, db-migrations) cover output formatting. Assessed against the PR description and TOON's stated goal (compact, safely-parseable output for LLM/script consumption).


Critical

1. Value-quoting regex closes its character class early → delimiter and structural chars are never quoted (src/lib/output.ts:23)

/[:,"\\{}[\\]]/.test(s)

The intent is "quote if the value contains any of : , " \ { } [ ]". But inside the class, \\ is a literal backslash and the very next ] closes the class. So the pattern actually parses as [:,"\{}[\] followed by a trailing literal ] — i.e. it only matches when one of those chars is immediately followed by a ]. Verified:

"a,b"        -> match: false   // comma NOT detected
"a:b"        -> match: false
'a"b'        -> match: false
"a{b}c"      -> match: false
"Smith, John"-> match: false
":]"         -> match: true    // only the accidental trailing-] form matches

Consequence: any value containing a comma (the TOON row delimiter) is emitted unquoted, so the row gains phantom columns and the output no longer parses. This is not hypothetical — it hits the PR's own advertised example. functions list builds the Created At cell via new Date(f.createdAt).toLocaleString() (src/commands/functions/list.ts:45), which in en-US yields e.g. 7/30/2026, 10:00:00 AM (contains a comma). Actual output:

[3]{Slug,Name,Status,"Created At"}:
  func-1,My Function,active,7/30/2026, 10:00:00 AM   <- 5 fields for a 4-col header

whereas the PR body shows the timestamp correctly quoted ("2026-07-30, 10:00:00 AM") — output the current code does not produce. Since the whole point of --toon is safely machine-parseable output, this defeats the feature and silently corrupts data.

Fix: escape the closing bracket so it stays inside the class, e.g. /[:,"\\{}[\]]/ (verified: this correctly quotes Smith, John). Please also cover ", \, {, }, [, ] while you're at it — all currently slip through the same way.


Suggestion

  • Software engineering — no tests added (src/lib/output.ts). This module is regex- and edge-case-heavy, and the repo has a strong test culture (600+ tests, one *.test.ts per lib module). A one-line assertion like expect(toonEscapeValue('a,b')).toMatch(/^"/) would have caught the Critical above immediately. Please add a src/lib/output.test.ts covering: delimiter/structural-char quoting, empty string, numeric-looking strings, booleans/null, control chars, leading/trailing whitespace, [N]{keys}: tabular framing, the [0]{keys}: empty case, single-object key/value, and the --json --toon path. The insforge-development skill requires every new behavior to carry at least one test.
  • Functionality — heterogeneous arrays (src/lib/output.ts:102). outputJsonToon derives columns from Object.keys(first) only; if later array elements have different keys, extra keys are dropped and missing ones render as null. Fine for uniform list endpoints, but worth a guard or a comment documenting the assumption.

Information

  • --json --toon precedence (src/lib/output.ts:50). When both flags are passed, TOON silently wins and --json is ignored. The PR body uses whoami --json --toon as an intended combo, so this is presumably deliberate — worth a note in --help so callers aren't surprised.
  • Nested objects (src/lib/output.ts:120-123). In single-object mode, nested objects are rendered as raw JSON.stringify(v) (embedded JSON, not TOON). Acceptable, but not strictly spec-conformant if downstream expects pure TOON.
  • toonEscapeKey (src/lib/output.ts:37-45) doesn't apply the control-char \uXXXX catch-all that toonEscapeValue does. Keys come from code-controlled headers so the risk is negligible today.

Security

No security-relevant changes — output-only path, no new user input reaching SQL/shell/HTTP, no new dependencies (formatting is hand-rolled), and TOON suppresses outputSuccess/outputInfo without touching secret handling.

Performance

No concerns — string concatenation over CLI-sized result lists, no DB queries, no hot path or event-loop blocking introduced.


Verdict: request_changes

One Critical correctness bug (the quoting regex) blocks merge; it corrupts the exact output the feature exists to produce and lands untested. The rest is a solid, well-contained change — fixing the regex and adding a focused output.test.ts should be enough to land it.

@jwfing

jwfing commented Jul 30, 2026

Copy link
Copy Markdown
Member

@akb4q could you please address these comments? thanks!

- Replace ad-hoc TOON serializer with official @toon-format/toon encode()
  (fixes nested structure loss, heterogeneous array crashes, null handling)
- Make --toon standalone: getRootOpts returns json=true when --toon set
  (fixes empty output and credential loss without --json)
- outputSuccess/outputInfo emit structured TOON in toonMode
  (fixes silent credential suppression)

Fixes greptile P1 (credential loss, nested structure),
coderabbit (use official encode API),
cubic-dev-ai P1/P2 (heterogeneous arrays, null drop, standalone mode)
@akb4q

akb4q commented Jul 30, 2026

Copy link
Copy Markdown
Author

@jwfing all review comments addressed:

  • Official encode: Replaced ad-hoc serializer with @toon-format/toon encode() API — handles nested structures, heterogeneous arrays, and null values correctly
  • 🔧 --toon standalone: --toon now works without --jsongetRootOpts() routes to structured output automatically (commands like whoami --toon, secrets rotate --toon produce proper TOON output)
  • 🔐 One-time credentials: outputSuccess/outputInfo emit structured TOON instead of suppressing — no more silent credential loss on secrets rotate or s3-keys create

Build ✅, ESLint 0 errors ✅, 648/649 tests passing ✅ (1 pre-existing Cloudflare flake)

Comment thread src/lib/output.ts Outdated

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All reported issues were addressed across 4 files (changes from recent commits).

Tip: Review your code locally with the cubic CLI to iterate faster.

Re-trigger cubic

Comment thread src/lib/output.ts Outdated
…tests

- outputSuccess/outputInfo: suppress in TOON mode (route through outputJson)
- Fix toonEscapeValue regex: /[]:,"\{}[]/ -> /[\]:,"\{}[]/ (\] escape fixes
  premature class close — commas now detected as quote-trigger chars)
- Add output.test.ts with 13 tests covering: structural-char quoting,
  empty string, boolean-likes, null handling, single-doc output,
  suppress behavior, tabular format, empty-table edge case
@akb4q

akb4q commented Jul 30, 2026

Copy link
Copy Markdown
Author

@jwfing addressed your review and the new greptile re-review:

  • outputSuccess/outputInfo now suppress in TOON mode. The getRootOpts() route (--toon implies json: true) ensures credential commands like secrets rotate --toon go through outputJson(result) which produces exactly one TOON document. No more multi-document stream.
  • Quoting regex fixed. The original /[:,"\\{}[\]]/ had ] closing the character class early, so commas and colons were never detected as quote triggers. Fixed to /[\]:,"\\{}[]/ using proper \] escape. Verified: Smith, John, 7/30/2026, 10:00:00 AM are now correctly quoted.
  • Added src/lib/output.test.ts with 13 tests: structural-char quoting, empty string, boolean-likes, null preservation, single-doc output, suppress behavior, tabular format, empty-table edge case.

Build passes, ESLint 0 errors, 661/662 tests passing (1 pre-existing Cloudflare flake).

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 `@src/lib/output.test.ts`:
- Line 46: Update the matcher in the output test around logs[0] to replace the
literal repeated spaces with a regex quantifier while preserving the expected
two-space indentation and trailing newline behavior.
🪄 Autofix

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: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 03c181e9-19f4-4dc3-9913-2f1df89fbf07

📥 Commits

Reviewing files that changed from the base of the PR and between 0a96fe2 and 0b66a55.

⛔ Files ignored due to path filters (1)
  • package-lock.json is excluded by !**/package-lock.json
📒 Files selected for processing (4)
  • package.json
  • src/index.ts
  • src/lib/output.test.ts
  • src/lib/output.ts
🚧 Files skipped from review as they are similar to previous changes (2)
  • package.json
  • src/index.ts

Comment thread src/lib/output.test.ts
setToonMode(true);
outputTable(['Name'], [['Alice']]);
// Tabular TOON: header on first line, values on second line starting with 2 spaces
expect(logs[0]).toMatch(/\n Alice\n?$/);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win

Fix the ESLint error in the matcher.

Line 46 uses literal repeated spaces in a regular expression. no-regex-spaces reports this as an error. Use {2} to preserve the expected TOON indentation.

Proposed fix
-      expect(logs[0]).toMatch(/\n  Alice\n?$/);
+      expect(logs[0]).toMatch(/\n {2}Alice\n?$/);
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
expect(logs[0]).toMatch(/\n Alice\n?$/);
expect(logs[0]).toMatch(/\n {2}Alice\n?$/);
🧰 Tools
🪛 ESLint

[error] 46-46: Spaces are hard to count. Use {2}.

(no-regex-spaces)

🤖 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 `@src/lib/output.test.ts` at line 46, Update the matcher in the output test
around logs[0] to replace the literal repeated spaces with a regex quantifier
while preserving the expected two-space indentation and trailing newline
behavior.

Source: Linters/SAST tools

@jwfing jwfing left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary
This PR is close, but the current implementation does not make --toon a truly global structured-output mode.

Requirements context
I did not find a linked issue or existing repo docs for TOON itself, so I assessed intent from the PR description plus existing repo conventions. The description says --toon is a global output flag with "zero changes to any command handler," and DEVELOPMENT.md says structured output should flow through outputJson / outputTable / outputSuccess in src/lib/output.ts.

Findings
Critical

  • src/commands/login.ts:99-107, src/commands/login.ts:123-140, src/commands/login.ts:185-189, src/commands/config/plan.ts:69-70, src/commands/config/export.ts:78-79: --toon is implemented by toggling module state that only outputJson / outputTable consult (src/lib/output.ts, src/index.ts). These command paths still call console.log(JSON.stringify(...)) directly, so insforge login --toon, insforge config plan --toon, and insforge config export --toon will continue to emit JSON instead of TOON. That breaks the PR's core contract that --toon is a global structured-output format and also violates the repo's output convention in DEVELOPMENT.md.

Suggestion

  • src/lib/errors.ts:95-107, src/lib/output.test.ts:1-214: error rendering still keys only off json and always prints JSON on structured error paths, so --toon failures will not be TOON-encoded. Separately, the new tests only exercise src/lib/output.ts in isolation and do not cover command-level --toon flows or error paths, which is why the direct JSON.stringify call sites above slipped through.

Information

  • Software engineering: no additional style/convention issues beyond the direct-output bypass above.
  • Security: no security-relevant changes beyond adding a pinned npm dependency in the lockfile.
  • Performance: no material performance issues in the new TOON helpers; the added work is small relative to existing CLI rendering.

Verdict
request_changes because the global --toon behavior is not implemented consistently across existing structured command outputs.

@jwfing jwfing left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please address the latest comment。

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-issue PR isn't linked to any issue — open and claim an issue first, then link it

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants