Open
Conversation
rohoswagger
changed the title
…d non-interactive configure Phase 1 of agent-friendly CLI improvements: - ask: support piped stdin input with --prompt flag for separating question from context (arg+stdin, prompt+stdin, stdin-only all work) - ask: auto-truncate output at 4KB when stdout is not a TTY (agent calling), save full response to temp file with exploration hints. --max-output to override threshold, --max-output 0 to disable - configure: accept --server-url and --api-key flags for non-interactive setup (tests connection before saving) - All commands: actionable error messages with remediation hints - All commands: help examples added to every subcommand - New unit tests for resolveQuestion and overflowWriter Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Phase 2 of agent-friendly CLI improvements: - New exitcodes package with typed ExitError: agents can distinguish not-configured (2), auth failure (3), unreachable (4), bad request (5) - main.go extracts exit codes via errors.As for proper os.Exit - All commands updated to return typed ExitError where appropriate - ask: add --quiet/-q flag to buffer output and print once at end (no streaming chunks, useful for scripting) - Unit tests for exitcodes package and quiet mode overflow writer Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Allows testing a server URL and API key combination without saving the config. Useful for agents and scripts that want to validate credentials before committing to a configuration change. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Review fixes: - ask: limit stdin reads to 10MB to prevent OOM on large pipes - serve: remove duplicate examples from Long description (Cobra renders the Example field separately) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
rohoswagger
force-pushed
the
cli-agent-friendly
branch
from
7d89401 to
718227a
Compare
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
3 issues found across 8 files
Confidence score: 3/5
- There is a concrete user-facing regression risk in
cli/cmd/ask.go: combining--jsonwith--quietcan return success with empty stdout, which can break automation expecting JSON events. cli/cmd/ask.goalso still buffers full responses before truncation, so long outputs may cause significant memory growth under real usage.cli/cmd/configure.gomay accidentally persistONYX_PERSONA_IDviaconfig.Load(), creating unexpected config state by saving env-derived values as defaults.- Pay close attention to
cli/cmd/ask.goandcli/cmd/configure.go- output semantics and config persistence behavior need validation before merge.
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="cli/cmd/configure.go">
<violation number="1" location="cli/cmd/configure.go:70">
P2: Using `config.Load()` here can persist `ONYX_PERSONA_ID` into the saved config. Read the existing value from disk-only config (without env overrides) before preserving `DefaultAgentID`.</violation>
</file>
<file name="cli/cmd/ask.go">
<violation number="1" location="cli/cmd/ask.go:113">
P1: `--json` + `--quiet` currently drops all output. JSON mode should still emit events (or reject the flag combination) instead of returning success with empty stdout.</violation>
<violation number="2" location="cli/cmd/ask.go:216">
P2: Truncation still buffers full responses in memory, which can cause large memory growth on long outputs.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Contributor
|
Preview Deployment
|
Every command now has a Long description for --help consistency. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Using both flags silently dropped all JSON events, returning exit 0 with empty stdout. Now rejects the combination upfront with a clear error. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
overflowWriter now opens a temp file eagerly when limit > 0 and streams chunks directly to disk. Previously it accumulated the full response in a strings.Builder, which could cause large memory growth on long outputs. The in-memory buf is now only used for quiet mode. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
Greptile SummaryThis PR makes the Key changes:
Issues found:
Confidence Score: 4/5Safe to merge after addressing the two P1 issues: truncation metadata going to stdout and stdin consumed before interactive wizard. Two P1 findings remain: (1) the truncation notice goes to stdout instead of stderr, directly undermining the agent-friendly goal of this PR by mixing metadata into the response stream, and (2) piping empty stdin to configure without flags consumes stdin before onboarding.Run is called, breaking the interactive wizard. Both are correctness issues on changed code paths. All prior review thread concerns (missing exitcodes package, prompt conflict, AuthFailure unreachable) are addressed in this revision. cli/internal/overflow/writer.go (truncation notice to stdout) and cli/cmd/configure.go (stdin consumed before interactive fallback) Important Files Changed
Sequence DiagramsequenceDiagram
participant Caller as Agent / Script
participant main as main.go
participant ask as cmd/ask.go
participant ow as overflow.Writer
participant api as api.Client
participant tmp as Temp File
Caller->>main: onyx-cli ask [question]
main->>ask: RunE()
ask->>ask: resolveQuestion(args, --prompt, stdin)
ask->>api: SendMessageStream(ctx, question)
api-->>ask: event channel
loop stream events
ask->>ow: Write(content)
ow->>tmp: WriteString(full content)
alt written < Limit
ow-->>Caller: fmt.Print(content) to stdout
else over Limit
ow->>ow: truncated = true
end
end
ask->>ow: Finish()
alt not truncated
ow->>tmp: Remove (cleanup)
ow-->>Caller: fmt.Println() to stdout
else truncated
ow-->>Caller: response truncated notice to stdout WRONG
ow-->>Caller: Full response path to stdout WRONG
Note over ow,Caller: Metadata should go to stderr
end
alt ExitError returned
main->>main: errors.As exitErr.Code
main->>Caller: os.Exit(code)
else generic error
main->>Caller: os.Exit(1)
end
Prompt To Fix All With AIThis is a comment left during a code review.
Path: cli/internal/overflow/writer.go
Line: 100-104
Comment:
**Truncation metadata written to stdout contaminates agent output**
The truncation notice and exploration hints are printed with `fmt.Printf` to stdout. For the stated agent-friendly goal, this is self-defeating: an AI agent reading stdout captures both the response content and the metadata as a single stream, making it impossible to cleanly separate the actual answer from the "file was truncated" advisory.
All progress/status messages in the stream handler (`SearchStartEvent`, `SearchQueriesEvent`, etc.) already correctly use `fmt.Fprintf(os.Stderr, ...)`. The truncation footer should follow the same pattern:
```go
fmt.Fprintf(os.Stderr, "\n\n--- response truncated (%d bytes total) ---\n", w.totalBytes)
fmt.Fprintf(os.Stderr, "Full response: %s\n", tmpPath)
fmt.Fprintf(os.Stderr, "Explore:\n")
fmt.Fprintf(os.Stderr, " cat %s | grep \"<pattern>\"\n", tmpPath)
fmt.Fprintf(os.Stderr, " cat %s | tail -50\n", tmpPath)
```
With this change, `stdout` stays clean machine-readable text and `stderr` carries the advisory, which is consistent with standard Unix tool conventions and the rest of the PR.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: cli/cmd/configure.go
Line: 52-74
Comment:
**Stdin consumed before interactive fallback makes onboarding wizard hang on EOF**
When stdin is piped but neither `--server-url` nor `--api-key` is provided (e.g. `echo "" | onyx-cli configure` or `cat /dev/null | onyx-cli configure`), the logic at line 52 detects a non-TTY stdin and calls `io.ReadAll(os.Stdin)`, consuming the entire stream. After trim, `apiKey` is `""`, and since both `serverURL` and `apiKey` are empty, none of the early-return guards fire and execution falls through to `onboarding.Run(&cfg)`.
`onboarding.Run` drives an interactive prompt wizard that reads from stdin — but stdin is now at EOF. Depending on the prompt library, this either panics, loops infinitely reading empty input, or silently produces a corrupt config.
The simplest fix is to guard stdin reading behind the condition that `serverURL` is already provided (i.e., we're actually attempting a non-interactive flow):
```go
// Only read API key from stdin when --server-url was also given or --api-key-stdin was explicit.
// Unconditionally consuming stdin breaks the interactive wizard if the user runs
// `echo "" | onyx-cli configure` without flags.
if apiKeyStdin || (apiKey == "" && serverURL != "" && !term.IsTerminal(int(os.Stdin.Fd()))) {
data, err := io.ReadAll(os.Stdin)
...
}
```
This ensures the interactive path always has a live stdin.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: cli/cmd/configure.go
Line: 53
Comment:
**No stdin size cap when reading the API key**
`io.ReadAll(os.Stdin)` has no upper bound. `ask.go` guards the equivalent stdin read with a 10 MB `io.LimitReader`. An API key is always tiny, so in practice this is harmless, but for consistency and defensive coding the same pattern should be applied here — even a small limit like 64 KB would prevent an accidental `cat large_file | onyx-cli configure` from allocating unbounded memory:
```suggestion
data, err := io.ReadAll(io.LimitReader(os.Stdin, 64*1024))
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: cli/internal/overflow/writer_test.go
Line: 19-29
Comment:
**Temp file leaked by `TestWriter_UnderLimit`**
When `Limit > 0`, the very first `Write` call creates a temp file (`onyx-ask-*.txt` under the system temp directory). `TestWriter_UnderLimit` writes 11 bytes with `Limit: 100`, so a temp file is created — but the test never calls `Finish()` or manually cleans it up. Every run of this test leaves an orphaned file in `/tmp`.
The other tests that exercise truncation (`TestWriter_OverLimit`, `TestWriter_MultipleChunks`) both close and remove the temp file explicitly. Apply the same treatment here:
```go
func TestWriter_UnderLimit(t *testing.T) {
w := &Writer{Limit: 100}
w.Write("hello")
w.Write(" world")
// Clean up the temp file created by the first Write
if w.tmpFile != nil {
name := w.tmpFile.Name()
_ = w.tmpFile.Close()
_ = os.Remove(name)
}
if w.truncated {
t.Fatal("should not be truncated when under limit")
}
if w.written != 11 {
t.Fatalf("expected 11 written bytes, got %d", w.written)
}
}
```
How can I resolve this? If you propose a fix, please make it concise.Reviews (6): Last reviewed commit: "fix(cli): suppress status lines in quiet..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
cli/cmd/ask.go
Outdated
Comment on lines
+214
to
+218
| // Quiet mode: buffer everything, print nothing | ||
| if w.quiet { | ||
| w.buf.WriteString(s) | ||
| return | ||
| } |
Contributor
There was a problem hiding this comment.
When --quiet is active, Write() accumulates all content in w.buf without any size check, and Finish() prints w.buf.String() in full — completely bypassing the limit field and any truncateAt threshold. This means that when an agent/script runs onyx-cli ask --quiet ..., the auto-enabled 4 KB truncation safety guard is silently ignored, and the full (potentially multi-MB) response is dumped to stdout in one shot.
This is the opposite of the agent-friendly goal described in the PR. The quiet mode accumulation path should still respect the limit:
func (w *overflowWriter) Finish() {
if w.quiet {
out := w.buf.String()
if w.limit > 0 && len(out) > w.limit {
// truncate and write to temp file, same as non-quiet path
// ...
} else {
fmt.Print(out)
}
return
}
// ...
}Prompt To Fix With AI
This is a comment left during a code review.
Path: cli/cmd/ask.go
Line: 214-218
Comment:
**Quiet mode bypasses non-TTY truncation limit**
When `--quiet` is active, `Write()` accumulates all content in `w.buf` without any size check, and `Finish()` prints `w.buf.String()` in full — completely bypassing the `limit` field and any `truncateAt` threshold. This means that when an agent/script runs `onyx-cli ask --quiet ...`, the auto-enabled 4 KB truncation safety guard is silently ignored, and the full (potentially multi-MB) response is dumped to stdout in one shot.
This is the opposite of the agent-friendly goal described in the PR. The quiet mode accumulation path should still respect the limit:
```go
func (w *overflowWriter) Finish() {
if w.quiet {
out := w.buf.String()
if w.limit > 0 && len(out) > w.limit {
// truncate and write to temp file, same as non-quiet path
// ...
} else {
fmt.Print(out)
}
return
}
// ...
}
```
How can I resolve this? If you propose a fix, please make it concise.configureNonInteractive used config.Load() which applies ONYX_PERSONA_ID env overrides, causing the env value to leak into the saved config file. Now uses LoadFromDisk() to read only persisted values. Also extracts LoadFromDisk as a reusable function from Load. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The exitcodes package was created on disk but never staged/committed because ez commit -am only picks up tracked files. Adding the new untracked files that all commands depend on. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
2 issues found across 5 files (changes from recent commits).
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="cli/cmd/ask.go">
<violation number="1" location="cli/cmd/ask.go:241">
P2: Handle temp-file write failures instead of silently discarding them; otherwise truncation mode can claim a full response file that was not actually written.
(Based on your team's feedback about warning instead of silently swallowing runtime behavior changes.) [FEEDBACK_USED]</violation>
</file>
<file name="cli/internal/exitcodes/codes.go">
<violation number="1" location="cli/internal/exitcodes/codes.go:36">
P2: `Wrap` allows `nil` errors, which can panic when `Error()` is called on the resulting `ExitError`. Guard `nil` in `Wrap` so `ExitError.Err` is always non-nil.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Passing both a positional argument and --prompt silently dropped the --prompt value. Now fails with a clear error. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
🖼️ Visual Regression Report
|
…Wrap - overflowWriter: check WriteString error and fall back to streaming directly to stdout instead of silently producing a truncated temp file - exitcodes.Wrap: guard nil error to prevent panic in ExitError.Error() Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
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="cli/cmd/ask.go">
<violation number="1" location="cli/cmd/ask.go:252">
P2: Clearing `w.truncated` on temp-file write failure can silently hide already-truncated output and produce partial/misaligned stdout without a truncation notice.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
jmelahman
approved these changes
Apr 2, 2026
cli/internal/exitcodes/codes.go
Outdated
| const ( | ||
| Success = 0 | ||
| General = 1 | ||
| NotConfigured = 2 |
Contributor
There was a problem hiding this comment.
I would make BadRequest two. Convention is typically to use 2 for invalid args/command line errors
cli/internal/exitcodes/codes.go
Outdated
| return e.Err.Error() | ||
| } | ||
|
|
||
| func (e *ExitError) Unwrap() error { |
Contributor
There was a problem hiding this comment.
Wrap and Unwrap are only used in tests. Maybe we can remove?
cli/cmd/configure.go
Outdated
| return &cobra.Command{ | ||
| var ( | ||
| serverURL string | ||
| apiKey string |
Contributor
There was a problem hiding this comment.
Does this support reading from stdin? It's fairly common to use files/env to avoid leaking api keys in shell history (I would recommend this/put it in the docs as it is more secure). For example, docker has both a --password and --password-stdin, https://docs.docker.com/reference/cli/docker/login/#options
Also, this should probably respect ONYX_API_KEY if it doesn't already
Contributor
Author
There was a problem hiding this comment.
yep added support for pipe, ONYX_API_KEY, and --api-key-stdin
cli/cmd/ask.go
Outdated
| // When limit > 0, it streams to a temp file on disk (not memory) and stops | ||
| // writing to stdout after limit bytes. When limit == 0, it writes directly | ||
| // to stdout. In quiet mode, it buffers in memory and prints once at the end. | ||
| type overflowWriter struct { |
Contributor
There was a problem hiding this comment.
nit: could maybe be moved to a dedicated package. Could be use for future commands and I find it better to refactor before LLMs duplicate it everywhere because they don't know to look in here -- they're much more willing to look at the file tree and especially utils than they are to read whole files.
cli/cmd/ask.go
Outdated
|
|
||
| // Close the temp file so it's readable | ||
| tmpPath := w.tmpFile.Name() | ||
| _ = w.tmpFile.Close() |
Contributor
There was a problem hiding this comment.
It's probably not a big deal, but we probably should be 1. defering closing files (so it closes even if this function is exited prematurely) and 2. at least logging when files fail to close. I don't think it's likely, but in an agentic world, this may manifest as too many files open errors eventually.
- exitcodes: swap BadRequest to code 2 (convention for invalid args), shift NotConfigured/AuthFailure/Unreachable to 3/4/5 - exitcodes: remove Wrap/Unwrap (only used in tests, per reviewer) - configure: add --api-key-stdin to read key from pipe instead of flag, avoiding shell history leaks (like docker login --password-stdin) - overflow: extract overflowWriter to internal/overflow package for reuse by future commands and better discoverability - overflow: use defer-based file cleanup with logged close errors - ask: guard ow.Finish() with !askJSON to fix spurious blank line appended to JSON event stream (Greptile P1) - ask: add arg+prompt conflict test Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Drop the --api-key-stdin flag. Instead, if --api-key is omitted and stdin has piped data, read the API key from stdin automatically. Simpler for agents — just pipe it: echo "$KEY" | onyx-cli configure --server-url https://... Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
When running in a terminal, ask now prints dim status lines to stderr
as the backend processes the request:
Searching documents...
→ what is our PTO policy
Found 5 documents
Thinking...
Status goes to stderr so it doesn't pollute stdout or interfere with
piping. Only shown when stdout is a TTY (agents don't see it).
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
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="cli/cmd/ask.go">
<violation number="1" location="cli/cmd/ask.go:133">
P2: `--quiet` mode still streams status updates to stderr; gate the new TTY progress prints behind `!askQuiet` so quiet mode remains non-streaming.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Stdin piping still works implicitly (just pipe without any flag), but --api-key-stdin makes the intent explicit and discoverable via --help. Errors if used together with --api-key. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
TestConnection now returns *api.AuthError for 401/403 responses. configure and validate-config check for this type and return exitcodes.AuthFailure (4) instead of exitcodes.Unreachable (5), so agents can distinguish "bad API key" from "server down". Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Status updates (searching, thinking, tool use) were printing to stderr even with --quiet. Now gated behind !askQuiet so quiet mode is truly non-streaming. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
1 issue found across 5 files (changes from recent commits).
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="cli/internal/api/client.go">
<violation number="1" location="cli/internal/api/client.go:152">
P2: Reverse-proxy 401/403 errors are incorrectly classified as `AuthError`, causing auth-failure exit codes instead of unreachable/connectivity exit codes.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| if resp2.StatusCode == 401 || resp2.StatusCode == 403 { | ||
| if isHTML || strings.Contains(respServer, "awselb") { | ||
| return fmt.Errorf("HTTP %d from a reverse proxy (not the Onyx backend).\n Check your deployment's ingress / proxy configuration", resp2.StatusCode) | ||
| return &AuthError{Message: fmt.Sprintf("HTTP %d from a reverse proxy (not the Onyx backend).\n Check your deployment's ingress / proxy configuration", resp2.StatusCode)} |
Contributor
There was a problem hiding this comment.
P2: Reverse-proxy 401/403 errors are incorrectly classified as AuthError, causing auth-failure exit codes instead of unreachable/connectivity exit codes.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At cli/internal/api/client.go, line 152:
<comment>Reverse-proxy 401/403 errors are incorrectly classified as `AuthError`, causing auth-failure exit codes instead of unreachable/connectivity exit codes.</comment>
<file context>
@@ -149,12 +149,12 @@ func (c *Client) TestConnection(ctx context.Context) error {
if resp2.StatusCode == 401 || resp2.StatusCode == 403 {
if isHTML || strings.Contains(respServer, "awselb") {
- return fmt.Errorf("HTTP %d from a reverse proxy (not the Onyx backend).\n Check your deployment's ingress / proxy configuration", resp2.StatusCode)
+ return &AuthError{Message: fmt.Sprintf("HTTP %d from a reverse proxy (not the Onyx backend).\n Check your deployment's ingress / proxy configuration", resp2.StatusCode)}
}
if resp2.StatusCode == 401 {
</file context>
Suggested change
| return &AuthError{Message: fmt.Sprintf("HTTP %d from a reverse proxy (not the Onyx backend).\n Check your deployment's ingress / proxy configuration", resp2.StatusCode)} | |
| return fmt.Errorf("HTTP %d from a reverse proxy (not the Onyx backend).\n Check your deployment's ingress / proxy configuration", resp2.StatusCode) |
Comment on lines
+100
to
+104
| fmt.Printf("\n\n--- response truncated (%d bytes total) ---\n", w.totalBytes) | ||
| fmt.Printf("Full response: %s\n", tmpPath) | ||
| fmt.Printf("Explore:\n") | ||
| fmt.Printf(" cat %s | grep \"<pattern>\"\n", tmpPath) | ||
| fmt.Printf(" cat %s | tail -50\n", tmpPath) |
Contributor
There was a problem hiding this comment.
The truncation notice and exploration hints are printed with fmt.Printf to stdout. For the stated agent-friendly goal, this is self-defeating: an AI agent reading stdout captures both the response content and the metadata as a single stream, making it impossible to cleanly separate the actual answer from the "file was truncated" advisory.
All progress/status messages in the stream handler (SearchStartEvent, SearchQueriesEvent, etc.) already correctly use fmt.Fprintf(os.Stderr, ...). The truncation footer should follow the same pattern:
fmt.Fprintf(os.Stderr, "\n\n--- response truncated (%d bytes total) ---\n", w.totalBytes)
fmt.Fprintf(os.Stderr, "Full response: %s\n", tmpPath)
fmt.Fprintf(os.Stderr, "Explore:\n")
fmt.Fprintf(os.Stderr, " cat %s | grep \"<pattern>\"\n", tmpPath)
fmt.Fprintf(os.Stderr, " cat %s | tail -50\n", tmpPath)With this change, stdout stays clean machine-readable text and stderr carries the advisory, which is consistent with standard Unix tool conventions and the rest of the PR.
Prompt To Fix With AI
This is a comment left during a code review.
Path: cli/internal/overflow/writer.go
Line: 100-104
Comment:
**Truncation metadata written to stdout contaminates agent output**
The truncation notice and exploration hints are printed with `fmt.Printf` to stdout. For the stated agent-friendly goal, this is self-defeating: an AI agent reading stdout captures both the response content and the metadata as a single stream, making it impossible to cleanly separate the actual answer from the "file was truncated" advisory.
All progress/status messages in the stream handler (`SearchStartEvent`, `SearchQueriesEvent`, etc.) already correctly use `fmt.Fprintf(os.Stderr, ...)`. The truncation footer should follow the same pattern:
```go
fmt.Fprintf(os.Stderr, "\n\n--- response truncated (%d bytes total) ---\n", w.totalBytes)
fmt.Fprintf(os.Stderr, "Full response: %s\n", tmpPath)
fmt.Fprintf(os.Stderr, "Explore:\n")
fmt.Fprintf(os.Stderr, " cat %s | grep \"<pattern>\"\n", tmpPath)
fmt.Fprintf(os.Stderr, " cat %s | tail -50\n", tmpPath)
```
With this change, `stdout` stays clean machine-readable text and `stderr` carries the advisory, which is consistent with standard Unix tool conventions and the rest of the PR.
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+52
to
74
| if (apiKey == "" && !term.IsTerminal(int(os.Stdin.Fd()))) || apiKeyStdin { | ||
| data, err := io.ReadAll(os.Stdin) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to read API key from stdin: %w", err) | ||
| } | ||
| apiKey = strings.TrimSpace(string(data)) | ||
| } | ||
|
|
||
| if serverURL != "" && apiKey != "" { | ||
| return configureNonInteractive(serverURL, apiKey, dryRun) | ||
| } | ||
|
|
||
| if dryRun { | ||
| return exitcodes.New(exitcodes.BadRequest, "--dry-run requires --server-url and --api-key") | ||
| } | ||
|
|
||
| if serverURL != "" || apiKey != "" { | ||
| return exitcodes.New(exitcodes.BadRequest, "both --server-url and --api-key are required for non-interactive setup\n Run 'onyx-cli configure' without flags for interactive setup") | ||
| } | ||
|
|
||
| cfg := config.Load() | ||
| onboarding.Run(&cfg) | ||
| return nil |
Contributor
There was a problem hiding this comment.
When stdin is piped but neither --server-url nor --api-key is provided (e.g. echo "" | onyx-cli configure or cat /dev/null | onyx-cli configure), the logic at line 52 detects a non-TTY stdin and calls io.ReadAll(os.Stdin), consuming the entire stream. After trim, apiKey is "", and since both serverURL and apiKey are empty, none of the early-return guards fire and execution falls through to onboarding.Run(&cfg).
onboarding.Run drives an interactive prompt wizard that reads from stdin — but stdin is now at EOF. Depending on the prompt library, this either panics, loops infinitely reading empty input, or silently produces a corrupt config.
The simplest fix is to guard stdin reading behind the condition that serverURL is already provided (i.e., we're actually attempting a non-interactive flow):
// Only read API key from stdin when --server-url was also given or --api-key-stdin was explicit.
// Unconditionally consuming stdin breaks the interactive wizard if the user runs
// `echo "" | onyx-cli configure` without flags.
if apiKeyStdin || (apiKey == "" && serverURL != "" && !term.IsTerminal(int(os.Stdin.Fd()))) {
data, err := io.ReadAll(os.Stdin)
...
}This ensures the interactive path always has a live stdin.
Prompt To Fix With AI
This is a comment left during a code review.
Path: cli/cmd/configure.go
Line: 52-74
Comment:
**Stdin consumed before interactive fallback makes onboarding wizard hang on EOF**
When stdin is piped but neither `--server-url` nor `--api-key` is provided (e.g. `echo "" | onyx-cli configure` or `cat /dev/null | onyx-cli configure`), the logic at line 52 detects a non-TTY stdin and calls `io.ReadAll(os.Stdin)`, consuming the entire stream. After trim, `apiKey` is `""`, and since both `serverURL` and `apiKey` are empty, none of the early-return guards fire and execution falls through to `onboarding.Run(&cfg)`.
`onboarding.Run` drives an interactive prompt wizard that reads from stdin — but stdin is now at EOF. Depending on the prompt library, this either panics, loops infinitely reading empty input, or silently produces a corrupt config.
The simplest fix is to guard stdin reading behind the condition that `serverURL` is already provided (i.e., we're actually attempting a non-interactive flow):
```go
// Only read API key from stdin when --server-url was also given or --api-key-stdin was explicit.
// Unconditionally consuming stdin breaks the interactive wizard if the user runs
// `echo "" | onyx-cli configure` without flags.
if apiKeyStdin || (apiKey == "" && serverURL != "" && !term.IsTerminal(int(os.Stdin.Fd()))) {
data, err := io.ReadAll(os.Stdin)
...
}
```
This ensures the interactive path always has a live stdin.
How can I resolve this? If you propose a fix, please make it concise.
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
2 participants
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.
Description
Makes the onyx-cli significantly more useful when invoked by AI agents and scripts, based on patterns from CLI-for-agents design articles.
Stdin support for
ask:cat error.log | onyx-cli ask --prompt "find the root cause"echo "what is onyx?" | onyx-cli askcat log | onyx-cli ask "summarize this"Auto-truncate for non-TTY callers:
grep,tail)--max-output Nto override threshold,--max-output 0to disableSemantic exit codes:
0success,2not configured,3auth failure,4unreachable,5bad requestexitcodespackage withExitErrortype, extracted inmain.goNon-interactive configure:
onyx-cli configure --server-url X --api-key Ysaves config without prompts--dry-runtests connection without savingOther improvements:
--quiet/-qflag onaskto buffer output and print once (no streaming)How Has This Been Tested?
resolveQuestion(3 cases),overflowWriter(5 cases including quiet mode),exitcodes(4 cases)go test ./...)go vetandgolangci-lintclean--helpoutput for all commandsAdditional Options
Summary by cubic
Make the CLI easier for agents and scripts with stdin-aware
ask, disk-backed auto-truncate, non-interactiveconfigure(implicit or explicit stdin via--api-key-stdin,--dry-run), quiet mode, semantic exit codes, and TTY-only streaming status lines (suppressed in--quiet).New Features
ask: accept piped stdin (works with arg or--prompt);--quietfor non-streaming; auto-truncate non-TTY output to 4KB by default (--max-output, 0 disables); full response saved to a temp file.ask: show dim search/reasoning/tool status on stderr while streaming, only when stdout is a TTY; suppressed with--quiet.configure: non-interactive via--server-urland--api-key, or read the API key from stdin when omitted;--api-key-stdinfor explicit piping; validates connection before saving;--dry-runto test without saving.exitcodes(2 bad request, 3 not configured, 4 auth failure, 5 unreachable) with clearer help and remediation hints.Bug Fixes
ask: cap stdin at 10MB; stream overflow directly to disk; reject--json+--quiet; reject conflicting arg +--prompt; avoid trailing newline in JSON output; fall back to stdout if temp file write fails.configure/validate-config: distinguish auth failures from unreachable via*api.AuthErrorand return codes 4 vs 5.configure: don’t persist env var overrides when saving.Written for commit 2c28b57. Summary will update on new commits.