Repository navigation
Add agentic JSON output mode for AI agent integration - #536
Conversation
github-actions
Bot
added
release-drafter:added
|
CI failures on the last run are addressed in commit
Validated locally: |
fabriciojs
left a comment
fabriciojs
left a comment
There was a problem hiding this comment.
Reviewed the agentic JSON output mode. I applied the patch to a scratch worktree: it builds, go vet is clean and go test ./... passes. go test -race -count=3 ./commands/... only surfaces the pre-existing TestVersionFlagCommand flake, which reproduces on main too — so the new sync.Mutex in FakeShell is doing its job.
The overall shape is good, but there are a few issues worth fixing before merge. Grouped by impact:
JSON contract is breakable
kool runcollapses every parse error into{"error":"script not found"}, so a malformedkool.ymlis misreported.kool status --output jsonemits nothing at all when there are no services.emitJSONErrorwrites JSON to stderr, thenmain.goappends a plain-texterror: ...line to the same stream, leaving stderr unparseable.
Data loss / potential hang
streamLogsJSONuses a defaultbufio.Scanner(64KB cap) and discardsscanner.Err(); one long log line silently ends streaming and leavescmd.Wait()blocked on an undrained pipe.- The non-follow JSON path uses
CombinedOutput, folding docker compose's own stderr warnings into the log stream as bogus entries.
Regressions to existing non-JSON behavior
- The new
useColor()inspectss.outStream, whichDefaultKoolTask.Runreplaces with anio.Pipewriter — so colored output inside long tasks (kool startetc.) silently goes plain. Fprintln(w, out...)andcolor.Sprint(out...)differ in how they separate operands, so spacing changes depending on whether color is on.- Color is decided from
outStreambut JSON-mode diagnostics go toerrStream, so--output json 2> filewrites ANSI codes into the file.
Polish
- JSON
servicesarray has no deterministic ordering. --outputsilently ignores anything that isn't exactlyjson.
Details inline. Thanks for putting this together — the feature itself is a nice addition.
| // we should just warn the user about multiple finds for the script | ||
| r.Shell().Warning("Attention: the script was found in more than one kool.yml file") | ||
| err = nil | ||
| } else if r.Shell().IsJSONOutput() { |
There was a problem hiding this comment.
This else if catches every error that isn't a typo-suggestion or a multiple-defined-script error, and rewrites it into ErrKoolScriptNotFound / {"error":"script not found"}.
So a malformed kool.yml, an unreadable file, or a YAML parse failure all get reported to the agent as a missing script — which is exactly the wrong signal, since an agent will then go looking for the script name rather than fixing the file.
Worth gating on the actual not-found case, e.g.:
} else if r.Shell().IsJSONOutput() {
if parser.IsScriptNotFoundError(err) {
r.emitJSONError("script not found", []string{})
err = ErrKoolScriptNotFound
} else {
r.emitJSONError(err.Error(), []string{})
}
return
}(or whatever the parser's not-found predicate is), so genuine parse errors keep their own message.
There was a problem hiding this comment.
Preserved genuine parser errors rather than replacing them with script-not-found. Added a command regression test and verified malformed YAML through the built CLI.
Pushed in e14e494. Validation: go test ./..., go vet ./..., and go test -race ./commands ./core/shell pass.
| return | ||
| } | ||
|
|
||
| scanner := bufio.NewScanner(stdout) |
There was a problem hiding this comment.
Two problems with this scanner in the follow path:
- 64KB token limit.
bufio.NewScannerdefaults tobufio.MaxScanTokenSize. A single log line longer than that makesScan()returnfalsewithbufio.ErrTooLong. That is not far-fetched for JSON-logging apps or stack traces. scanner.Err()is discarded. When the above happens the loop just exits,err = cmd.Wait()is called on a process whose stdout pipe is no longer being drained, and the command blocks once the pipe buffer fills. From the agent's point of viewkool logs -f --output jsonsilently stops emitting and hangs.
Suggest raising the buffer and checking the error:
scanner := bufio.NewScanner(stdout)
scanner.Buffer(make([]byte, 0, 64*1024), 1024*1024)
for scanner.Scan() {
...
}
if e := scanner.Err(); e != nil {
_ = cmd.Wait()
return e
}Or drop the scanner entirely for a bufio.Reader + ReadString('\n') loop, which has no line-length ceiling.
There was a problem hiding this comment.
Replaced Scanner with an unbounded buffered reader, handle read/write errors, and kill/reap the child on failure. Follow and non-follow tests cover 128 KiB lines and unterminated final lines.
Pushed in e14e494. Validation: go test ./..., go vet ./..., and go test -race ./commands ./core/shell pass.
|
|
||
| s.table.SetWriter(s.Shell().OutStream()) | ||
| s.table.AppendHeader("Service", "Running", "Ports", "State") | ||
| if !s.Shell().IsJSONOutput() { |
There was a problem hiding this comment.
The len(services) == 0 early return a few lines above (L83-86) fires before any JSON handling, so it calls Warning("No services found.") and returns with nothing written to stdout.
Result: kool status --output json on a project with no services emits an empty stdout. A consumer doing kool status --output json | jq gets a parse error rather than the perfectly valid {"services":[],"count":0}.
Since the JSON struct already initialises Services with make(..., 0, len(statuses)), emitting the empty document is the natural behavior. Worth moving the JSON branch above the early return, or special-casing it there.
There was a problem hiding this comment.
Both legacy and workspace status paths now emit an empty services array and count 0. Added command-level coverage.
Pushed in e14e494. Validation: go test ./..., go vet ./..., and go test -race ./commands ./core/shell pass.
| // Color is disabled when NO_COLOR env is set (handled by gookit/color) | ||
| // or when the output stream is not a terminal. | ||
| func (s *DefaultShell) useColor() bool { | ||
| return color.Enable && NewTerminalChecker().IsTerminal(s.outStream) |
There was a problem hiding this comment.
This checks s.outStream directly rather than going through a stream that reflects the current task context — and DefaultKoolTask.Run swaps outStream for an io.Pipe writer for the entire duration of the task.
An io.Pipe writer is never a terminal, so IsTerminal returns false and all colored Warning / Success / Info / Error output produced inside a long-running task (kool start, kool preset, etc.) loses its color, even on a fully interactive TTY. That's a visible regression for existing non-JSON users, not just a JSON-mode concern.
The pre-existing code colored unconditionally, which is why this didn't come up before. Probably needs to consult the original/underlying stream (or cache the TTY decision at shell construction time, before any task swaps the stream).
There was a problem hiding this comment.
Preserved the underlying terminal stream when task output is temporarily redirected through io.Pipe. Added a PTY regression test.
Pushed in e14e494. Validation: go test ./..., go vet ./..., and go test -race ./commands ./core/shell pass.
| "suggestions": suggestions, | ||
| } | ||
| errPayload, _ := json.Marshal(payload) | ||
| _, _ = fmt.Fprintln(r.Shell().ErrStream(), string(errPayload)) |
There was a problem hiding this comment.
emitJSONError writes the JSON payload to ErrStream(), but the error is still returned up the stack and main.go ends up calling Shell().Error(err), which — in JSON mode — also writes to stderr (via diagnosticStream()).
So stderr ends up as:
{"error":"script not found","suggestions":[]}
error: script not found
A consumer that reasonably assumes "stdout is data, stderr is JSON diagnostics" can't parse that trailing line. Either send the structured error to stdout (keeping stderr free-form), or suppress the plain-text Error() call when the JSON payload was already emitted.
There was a problem hiding this comment.
Structured run errors are marked as already reported, and shell error rendering skips a second diagnostic. Other JSON-mode errors render as JSON. Verified the built CLI emits exactly one JSON document for typo, absent-script, and malformed-YAML failures.
Pushed in e14e494. Validation: go test ./..., go vet ./..., and go test -race ./commands ./core/shell pass.
| if s.useColor() { | ||
| out = []interface{}{color.New(color.Yellow).Sprint(out...)} | ||
| } | ||
| _, _ = fmt.Fprintln(s.diagnosticStream(), out...) |
There was a problem hiding this comment.
The colored and uncolored paths don't format identically. When useColor() is true, the variadic operands are collapsed by color.Sprint(out...) (which concatenates with no separator between non-string operands beyond Go's Sprint rules); when it's false, Fprintln(w, out...) inserts a space between operands.
Verified empirically: Info("\t", cmd) produces "\tmyscript" with color enabled and "\t myscript" without. Callers that pass an explicit indent/prefix as a separate operand get different spacing depending on whether stdout is a TTY.
Same pattern applies to Success, Info, and Warning. Cleanest fix is to normalize once before branching:
msg := fmt.Sprint(out...)
if s.useColor() {
msg = color.New(color.Yellow).Sprint(msg)
}
_, _ = fmt.Fprintln(s.diagnosticStream(), msg)There was a problem hiding this comment.
Normalize diagnostic operands once with fmt.Sprint before either color path. Added explicit indentation/spacing coverage.
Pushed in e14e494. Validation: go test ./..., go vet ./..., and go test -race ./commands ./core/shell pass.
| Services: make([]statusServiceJSON, 0, len(statuses)), | ||
| Count: len(statuses), | ||
| } | ||
| for _, ss := range statuses { |
There was a problem hiding this comment.
statuses is appended in the order results arrive on chStatus, which is goroutine-completion order — so the JSON services array ordering varies between runs on the same project.
The table path doesn't have this problem because the renderer applies SortBy(1). For a machine-readable format the nondeterminism is worse than for a table: it breaks golden-file tests, diffing two kool status --output json runs, and any agent caching keyed on the output.
Suggest sorting statuses by service name before building the payload.
There was a problem hiding this comment.
Sort JSON status entries by project then service, including the legacy path, and retain workspace project identity. Added sorted-output coverage.
Pushed in e14e494. Validation: go test ./..., go vet ./..., and go test -race ./commands ./core/shell pass.
| if s.IsJSONOutput() { | ||
| return s.errStream | ||
| } | ||
| return s.outStream |
There was a problem hiding this comment.
diagnosticStream() correctly routes diagnostics to errStream in JSON mode, but useColor() (just below) still decides colorization from outStream. The two disagree exactly when they matter most.
Concretely: kool start --output json 2> log.txt leaves stdout on a TTY, so useColor() returns true and ANSI escape codes get written into the redirected log.txt. The inverse also happens — piping stdout while stderr stays a TTY drops color from messages that could have had it.
Color should be decided from the stream actually being written to, i.e. IsTerminal(s.diagnosticStream()) for the diagnostic helpers.
There was a problem hiding this comment.
Diagnostic color detection now uses the stderr terminal in JSON mode, respects NO_COLOR, and preserves terminal context for task pipes. PTY/redirect regression coverage added.
Pushed in e14e494. Validation: go test ./..., go vet ./..., and go test -race ./commands ./core/shell pass.
| } | ||
|
|
||
| var output string | ||
| if output, err = l.Shell().Exec(l.logs, args...); err != nil { |
There was a problem hiding this comment.
The non-follow path goes through Shell().Exec, which uses CombinedOutput — so docker compose's own stderr chatter (WARN[0000] ..., orphan-container notices, deprecation warnings) is interleaved into output and then fed line-by-line through parseLogLine, producing bogus log entries in the JSON stream.
The --follow path gets this right by taking only StdoutPipe(). Worth making the two consistent, so the same command with and without -f doesn't yield structurally different data.
There was a problem hiding this comment.
Follow and non-follow JSON logs now share the same stdout-only subprocess path; Compose stderr goes to diagnostics and --no-color is passed. Tests verify warnings are excluded in both modes.
Pushed in e14e494. Validation: go test ./..., go vet ./..., and go test -race ./commands ./core/shell pass.
| env.Set("KOOL_VERBOSE", verbose.Value.String()) | ||
| } | ||
|
|
||
| if output := cmd.Flags().Lookup("output"); output != nil && output.Value.String() == "json" { |
There was a problem hiding this comment.
The comparison is an exact match against "json", so anything else is silently ignored: --output JSON, --output jsonl, --output yaml, or a typo like --output jsno all fall through to normal human-readable output with no error.
For a flag whose entire purpose is machine consumption, silent fallback is a bad failure mode — the caller gets table output where it expected JSON and has to figure out why. Worth rejecting unknown values explicitly (and, if it's cheap, case-folding so --output JSON works).
There was a problem hiding this comment.
Validate explicit --output values as text or json (case-insensitive); unknown formats return an error. Added command-level tests.
Pushed in e14e494. Validation: go test ./..., go vet ./..., and go test -race ./commands ./core/shell pass.
|
Merged the newly updated main (including #540) and resolved the status-test conflict in 122e483. Final integration review also fixed duplicate diagnostics in JSON info when Compose is unavailable, normalized Compose V2 service-replica log prefixes, and preserved wrapped exit codes at the CLI boundary. Validation: go test ./..., go test -race ./commands ./core/shell, and go vet ./... pass. Added coverage for JSON status with Compose warning noise in both legacy/workspace paths and info error handling. Built the CLI and checked status/info/run/logs JSON against a running Bun app, plus malformed YAML and missing-Compose errors as single JSON documents. Smoke-test container/network removed afterward. Waiting for CI; security maintenance remains deferred as authorized. |
|
Published in 3.8.0, with all six standalone binaries and their SHA-256 files attached. Includes the reviewed JSON-contract, log-streaming, and terminal-output fixes. Thanks @danielsuguimoto! |
Description
Adds a global
--output jsonflag that enables machine-readable JSON output across kool commands, making the CLI suitable for use by AI agents and automation tools. When JSON mode is active, commands emit structured payloads instead of human-readable tables or text, diagnostics route to stderr to keep stdout clean for data, and interactive prompts are automatically disabled.Commands with JSON output:
kool status: emits{"services":[...],"count":N}with service state, ports, and running statuskool logs: emits JSON Lines format ({"service":"...","message":"..."}) with streaming support for--followkool info: emits structured payload with kool/docker versions, binary paths, and environment variables (KOOL_API_TOKEN is redacted)kool run: script listing works in JSON mode; script-not-found errors emit structured JSON to stderr with suggestions fieldThe existing
--jsonflag onkool runis now hidden but remains functional for backwards compatibility, unified under the global--output jsonflag.Notes
--output jsonis a global flag that must be placed before the script name when usingkool runkool run <script>remains raw (unstructured) as it passes through the underlying command output--jsonflag onkool runis hidden but still works for backwards compatibilitygo vetclean