Skip to content

feat(fetch): add --max-chars flag - #60

Merged
IlyaGusev merged 1 commit into
mainfrom
feat/fetch-max-chars
Aug 12, 2026
Merged

feat(fetch): add --max-chars flag#60
IlyaGusev merged 1 commit into
mainfrom
feat/fetch-max-chars

Conversation

@IlyaGusev

Copy link
Copy Markdown
Collaborator

Summary

  • Add --max-chars to keenable fetch. It maps to the API's max_chars query param and overrides the backend's default 50000-char content cap.
  • The flag flows through both paths (daemon and direct HTTP) via the shared fetch_query() builder.
  • Clap rejects 0 client-side; the backend accepts any positive integer.

Tests

  • cargo check and cargo clippy pass.
  • Smoke-tested against the live API: --max-chars 50 returns truncated content with the disclaimer; --max-chars 0 exits 2.
  • New e2e tests: test_fetch_max_chars (disclaimer assert catches a stale daemon that drops the param) and test_fetch_max_chars_rejects_zero.

🤖 Generated with Claude Code

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@qodo-code-review

Copy link
Copy Markdown

PR Summary by Qodo

fetch: add --max-chars flag to override fetch content cap

✨ Enhancement 🧪 Tests 📝 Documentation 🕐 20-40 Minutes

Grey Divider

AI Description

• Add --max-chars to keenable fetch to override the API’s default truncation cap.
• Thread max_chars through both daemon and direct HTTP via shared query building.
• Add e2e coverage and update CLI/README examples.
Diagram

graph TD
tests["e2e tests"] --> cli["keenable fetch CLI"] --> req["DaemonRequest (max_chars)"] --> exec["execute()"] --> query["fetch_query()"] --> http["Direct HTTP client"] --> api{{"Keenable API /v1/fetch"}}
req --> daemon["Local daemon"] --> query --> api
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Use `NonZeroU64` for `--max-chars`
  • ➕ Encodes the “must be > 0” constraint in the type system
  • ➕ Avoids repeating range validation logic if the value is reused elsewhere
  • ➖ Slightly more type conversion/ergonomics overhead when threading through Option/serde
  • ➖ Requires small signature/serde adjustments (e.g., Option or mapping to u64)
2. Introduce a typed fetch query struct + URL encoding
  • ➕ Avoids ad-hoc Vec assembly and reduces key/param drift risk further
  • ➕ Scales better as more query params are added
  • ➖ More code and abstractions than warranted for the current small parameter set
  • ➖ May require additional dependency or custom serialization glue

Recommendation: Current approach is good: keeping fetch_query() shared between daemon and direct HTTP is the right guardrail against parameter drift (and the new e2e test explicitly protects this). If the codebase adds more numeric constraints like this, consider switching max_chars to NonZeroU64 for stronger type-level validation.

Files changed (5) +37 / -7

Enhancement (3) +21 / -7
search.rsThread 'max_chars' into fetch DaemonRequest +2/-0

Thread 'max_chars' into fetch DaemonRequest

• Extends the 'fetch()' command handler signature to accept 'max_chars' and includes it in the constructed 'DaemonRequest' so it can reach both execution paths.

src/commands/search.rs

daemon.rsAdd 'max_chars' to daemon request + shared fetch query builder +11/-5

Add 'max_chars' to daemon request + shared fetch query builder

• Adds an optional 'max_chars' field to 'DaemonRequest' and updates 'fetch_query()' to emit an owned query vector including 'max_chars' when set.

src/daemon.rs

main.rsAdd '--max-chars' Clap flag and plumb to fetch handler +8/-2

Add '--max-chars' Clap flag and plumb to fetch handler

• Introduces the '--max-chars' option for 'keenable fetch' with client-side rejection of 0, updates help examples, and passes the value through to 'commands::search::fetch()'.

src/main.rs

Tests (1) +15 / -0
test_fetch.pyAdd e2e coverage for '--max-chars' and zero rejection +15/-0

Add e2e coverage for '--max-chars' and zero rejection

• Adds a fetch truncation test asserting the truncation disclaimer (to catch daemons that drop the param) and a parse-error test verifying '--max-chars 0' is rejected by Clap.

tests/e2e/test_fetch.py

Documentation (1) +1 / -0
README.mdDocument '--max-chars' usage in fetch examples +1/-0

Document '--max-chars' usage in fetch examples

• Adds a 'keenable fetch --max-chars ...' example to show how to raise the default 50k character cap.

README.md

@qodo-code-review

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (2) 📘 Rule violations (1) 📜 Skill insights (0)

Grey Divider


Remediation recommended

1. --max-chars error exits 2 📘 Rule violation ☼ Reliability
Description
--max-chars uses clap value validation (range(1..)) which causes invalid input (e.g., 0) to
exit with code 2, conflicting with the documented requirement that command errors exit with code 1.
The new e2e test codifies this non-1 exit behavior, making the error-handling contract harder to
standardize later.
Code

src/main.rs[R196-198]

+        /// Truncate content at this many characters (default: 50000)
+        #[arg(long = "max-chars", value_parser = clap::value_parser!(u64).range(1..))]
+        max_chars: Option<u64>,
Evidence
PR Compliance ID 7 requires commands to exit with code 1 on errors. The new --max-chars argument
is defined with a clap range validator, which triggers a clap parse failure (exit code 2) for 0,
and the newly-added e2e test explicitly asserts res.code == 2 for that case.

CLAUDE.md: Implement documented error handling: exit code 1, actionable hints, structured API/YAML errors
src/main.rs[196-198]
tests/e2e/test_fetch.py[51-54]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The new `--max-chars` flag is validated by clap (`range(1..)`) which produces a clap parse error exit code (2) on invalid values like `0`. Compliance requires errors to exit with code 1 and to emit actionable/structured errors.

## Issue Context
A new e2e test (`test_fetch_max_chars_rejects_zero`) asserts exit code 2 for `--max-chars 0`, reinforcing the non-compliant error code behavior.

## Fix Focus Areas
- src/main.rs[196-198]
- tests/e2e/test_fetch.py[51-54]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


2. Unbounded max_chars request 🐞 Bug ☼ Reliability
Description
--max-chars accepts any positive u64, so the CLI/daemon can request extremely large fetch responses.
Since fetch responses are fully buffered and parsed as JSON before printing, very large responses
can drive high memory usage or OOM when the API honors large max_chars values.
Code

src/main.rs[R196-198]

+        /// Truncate content at this many characters (default: 50000)
+        #[arg(long = "max-chars", value_parser = clap::value_parser!(u64).range(1..))]
+        max_chars: Option<u64>,
Evidence
The new CLI flag has no upper bound and is forwarded as the max_chars query param. The fetch code
path uses reqwest + handle_response, which parses the full HTTP body as JSON
(resp.json::<Value>()), implying responses are fully buffered in memory before output.

src/main.rs[179-199]
src/daemon.rs[30-49]
src/commands/search.rs[96-149]
src/api.rs[143-153]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`--max-chars` currently accepts any positive `u64` and is forwarded to the API. The fetch response is buffered and parsed into `serde_json::Value` before output, so very large responses can cause high memory usage or OOM.

## Issue Context
- The feature intent is to override the backend’s default cap (50,000 chars), so the fix should preserve legitimate increases (e.g. 200,000) while adding a safety guard.

## Fix Focus Areas
- Add a reasonable upper bound in clap parsing (or add a warning/confirmation above a threshold), consistent with expected product limits.
- Optionally document the bound in help/README.
- If large payloads are expected, consider a longer-term refactor to avoid fully buffering/parsing the response (streaming), but the immediate mitigation can be validation/guardrails.

### Code pointers
- src/main.rs[179-199]
- src/daemon.rs[30-49]
- src/commands/search.rs[96-149]
- src/api.rs[143-153]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Informational

3. Brittle truncation assertion 🐞 Bug ☼ Reliability
Description
The new e2e test hard-depends on a specific backend-generated truncation disclaimer phrase. If the
API changes wording while preserving behavior, the test will fail even though --max-chars plumbing
still works.
Code

tests/e2e/test_fetch.py[R46-49]

+    # The truncation disclaimer is the load-bearing assert: a stale daemon
+    # that drops `max_chars` returns the full page with exit code 0.
+    assert "truncated to stay below 50 characters" in data["content"]
+
Evidence
The test asserts that data["content"] contains the exact phrase "truncated to stay below 50
characters", which is backend wording and not a structural invariant.

tests/e2e/test_fetch.py[42-55]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`test_fetch_max_chars` asserts a specific disclaimer substring from live API output. This couples the test to backend prose and can cause false failures on harmless wording changes.

## Issue Context
The comment explains the assert is intended to detect a stale daemon that drops `max_chars`. That intent can still be met with more stable assertions.

## Fix Focus Areas
- Replace the exact-phrase match with a more robust invariant (e.g., content length is <= a small bound; or content contains a generic "truncated" marker; or (best) assert a structured field if the API provides one).

### Code pointers
- tests/e2e/test_fetch.py[42-55]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Tip of the day
💡 Did you know, you can group findings by type and pick your Finding display, from Minimal to Full

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Grey Divider

Qodo Logo

Comment thread src/main.rs
Comment on lines +196 to +198
/// Truncate content at this many characters (default: 50000)
#[arg(long = "max-chars", value_parser = clap::value_parser!(u64).range(1..))]
max_chars: Option<u64>,

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Remediation recommended

1. --max-chars error exits 2 📘 Rule violation ☼ Reliability

--max-chars uses clap value validation (range(1..)) which causes invalid input (e.g., 0) to
exit with code 2, conflicting with the documented requirement that command errors exit with code 1.
The new e2e test codifies this non-1 exit behavior, making the error-handling contract harder to
standardize later.
Agent Prompt
## Issue description
The new `--max-chars` flag is validated by clap (`range(1..)`) which produces a clap parse error exit code (2) on invalid values like `0`. Compliance requires errors to exit with code 1 and to emit actionable/structured errors.

## Issue Context
A new e2e test (`test_fetch_max_chars_rejects_zero`) asserts exit code 2 for `--max-chars 0`, reinforcing the non-compliant error code behavior.

## Fix Focus Areas
- src/main.rs[196-198]
- tests/e2e/test_fetch.py[51-54]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Comment thread src/main.rs
Comment on lines +196 to +198
/// Truncate content at this many characters (default: 50000)
#[arg(long = "max-chars", value_parser = clap::value_parser!(u64).range(1..))]
max_chars: Option<u64>,

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Remediation recommended

2. Unbounded max_chars request 🐞 Bug ☼ Reliability

--max-chars accepts any positive u64, so the CLI/daemon can request extremely large fetch responses.
Since fetch responses are fully buffered and parsed as JSON before printing, very large responses
can drive high memory usage or OOM when the API honors large max_chars values.
Agent Prompt
## Issue description
`--max-chars` currently accepts any positive `u64` and is forwarded to the API. The fetch response is buffered and parsed into `serde_json::Value` before output, so very large responses can cause high memory usage or OOM.

## Issue Context
- The feature intent is to override the backend’s default cap (50,000 chars), so the fix should preserve legitimate increases (e.g. 200,000) while adding a safety guard.

## Fix Focus Areas
- Add a reasonable upper bound in clap parsing (or add a warning/confirmation above a threshold), consistent with expected product limits.
- Optionally document the bound in help/README.
- If large payloads are expected, consider a longer-term refactor to avoid fully buffering/parsing the response (streaming), but the immediate mitigation can be validation/guardrails.

### Code pointers
- src/main.rs[179-199]
- src/daemon.rs[30-49]
- src/commands/search.rs[96-149]
- src/api.rs[143-153]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Comment thread tests/e2e/test_fetch.py
Comment on lines +46 to +49
# The truncation disclaimer is the load-bearing assert: a stale daemon
# that drops `max_chars` returns the full page with exit code 0.
assert "truncated to stay below 50 characters" in data["content"]

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Informational

3. Brittle truncation assertion 🐞 Bug ☼ Reliability

The new e2e test hard-depends on a specific backend-generated truncation disclaimer phrase. If the
API changes wording while preserving behavior, the test will fail even though --max-chars plumbing
still works.
Agent Prompt
## Issue description
`test_fetch_max_chars` asserts a specific disclaimer substring from live API output. This couples the test to backend prose and can cause false failures on harmless wording changes.

## Issue Context
The comment explains the assert is intended to detect a stale daemon that drops `max_chars`. That intent can still be met with more stable assertions.

## Fix Focus Areas
- Replace the exact-phrase match with a more robust invariant (e.g., content length is <= a small bound; or content contains a generic "truncated" marker; or (best) assert a structured field if the API provides one).

### Code pointers
- tests/e2e/test_fetch.py[42-55]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

@IlyaGusev
IlyaGusev merged commit 0d7618c into main Aug 12, 2026
12 checks passed
@IlyaGusev
IlyaGusev deleted the feat/fetch-max-chars branch August 12, 2026 10:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant