Repository navigation
fix(cli): errors to stderr, default fetch cap 20k chars - #118
Open
KGKallasmaa wants to merge 1 commit into
Open
KGKallasmaa wants to merge 1 commit into
KGKallasmaa wants to merge 1 commit into
Conversation
On a failed search or fetch, stdout is now empty and the structured YAML error goes to stderr (it opens with an `error:` line). Exit codes are unchanged. The invalid KEENABLE_API_URL error follows the same rule. fetch now sends max_chars=20000 when --max-chars is not given, matching the MCP server's default. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
PR Summary by QodoSend CLI errors to stderr and cap fetch output at 20,000 characters
AI Description
Diagram
High-Level Assessment
Files changed (11)
|
Code Review by Qodo
1. Verbose failures break error YAML
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+523
to
+524
| let yaml = serde_yaml::to_string(&val).unwrap_or_default(); | ||
| eprint!("{}", yaml); |
There was a problem hiding this comment.
1. Verbose failures break error yaml 🐞 Bug ≡ Correctness
handle_api_error prints a human-readable Error chain: before the newly redirected YAML when verbose mode is enabled and the error has multiple causes. A search -v or fetch -v transport failure then produces stderr that neither starts with error: nor parses as one YAML document.
Agent Prompt
## Issue description
Verbose transport errors print a human-readable cause chain before the YAML now written to stderr, breaking stderr parsing.
## Fix Focus Areas
- src/commands/search.rs[501-524]
## Recommended Fix
Do not emit separate verbose chain lines in YAML mode; retain the cause information in the structured YAML fields. Keep human-readable chain output in pretty mode.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+77
to
+80
| if data.get("total_chars", 0) <= 20000: | ||
| pytest.skip("page shorter than the cap") | ||
| assert len(data["content"]) <= 20000 | ||
| assert data["truncated"] is True |
There was a problem hiding this comment.
2. Nightly tests fail before the next release 🐞 Bug ☼ Reliability
test_fetch_default_cap_is_20k unconditionally requires behavior introduced in this PR, but the nightly workflow runs the checked-out tests against the latest published binary. Until a binary with the new default is released, a fetched page longer than 20,000 characters is subject to the older 50,000-character API default and fails this assertion.
Agent Prompt
## Issue description
The scheduled E2E suite runs current tests against the latest release, which does not yet have the new fetch default.
## Fix Focus Areas
- tests/e2e/test_fetch.py[72-80]
- tests/e2e/conftest.py[74-77]
## Recommended Fix
Detect whether the installed binary advertises the 20,000-character default and skip this particular default-cap assertion for older binaries; still run it against builds that advertise the new default.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+79
to
+80
| assert len(data["content"]) <= 20000 | ||
| assert data["truncated"] is True |
There was a problem hiding this comment.
3. Older truncation responses fail the new test 🐞 Bug ≡ Correctness
test_fetch_default_cap_is_20k requires the entire returned content to be at most 20,000 characters, although the older supported API appends a truncation notice beyond the requested cap. When that response format is returned for a long page, the test fails despite the CLI sending and the API applying the new default.
Agent Prompt
## Issue description
The new default-cap test rejects the older truncation response format already accepted by the explicit-cap test.
## Fix Focus Areas
- tests/e2e/test_fetch.py[51-80]
## Recommended Fix
Apply the neighboring test's legacy-notice handling to the 20,000-character assertion. For responses without that notice, retain the content-length and truncated-field checks.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+77
to
+78
| if data.get("total_chars", 0) <= 20000: | ||
| pytest.skip("page shorter than the cap") |
There was a problem hiding this comment.
4. Missing response data skips the cap test 🐞 Bug ☼ Reliability
test_fetch_default_cap_is_20k uses zero as the fallback for a missing total_chars and treats that value as evidence that the page is short. If the API omits that field for a long response, the suite skips its only assertion of the new default rather than detecting the missing data.
Agent Prompt
## Issue description
A missing `total_chars` is mistaken for a short page, so the new default-cap test can silently skip.
## Fix Focus Areas
- tests/e2e/test_fetch.py[76-80]
## Recommended Fix
Assert that `total_chars` exists and is numeric before using it to decide whether the response page is too short to test the cap.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Auto-approved: Qodo review completed — 🐞 Bugs (4),📘 Rule violations (0) 📜 Skill insights (0). Findings are advisory per team policy; verdict source: #118 (comment) [auto-approve.yml]
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
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.
What
Two fixes from a user's feedback (daily CLI + MCP user, scripts parsing stdout):
searchorfetch, stdout is now empty and the structured YAML error is written to stderr. It opens with anerror: …line, so a script that reads stderr sees the failure at once. Exit codes are unchanged (3 auth, 4 rate limit, 5 credits, 6 target, 7 network, 8 invalid request, 9 server). The invalidKEENABLE_API_URLerror follows the same rule.-poutput is unchanged.fetchdefault cap is 20,000 characters. The CLI sendsmax_chars=20000when--max-charsis not given, matching the MCP server.--max-charsstill overrides it. Before, the API default of 50,000 could overflow an agent's context.Breaking change
Scripts that parse the error YAML from stdout will break: read stderr instead. The one-line
error:summary that used to be a separate stderr line is now the first line of the YAML.Tests
On the devbox:
cargo build --release,cargo test --release(157 unit, 13api_url, 3tls_proxy) andcargo fmt --checkall pass. Live keyless runs of the built binary:error: Authentication failederror: Not founderror: Rate limit exceededtruncated: true)The pytest e2e suite needs
KEENABLE_API_KEYand was not run (no secrets on the devbox).Result.yaml()falls back to stderr on failure, so it works against old and new binaries (the nightly runs the latest release). New:test_fetch_default_cap_is_20kand a stdout-empty assertion intest_invalid_api_key.Follow-ups
🤖 Generated with Claude Code