diff --git a/.github/workflows/e2e.yml b/.github/workflows/e2e.yml index cc2b426..f802735 100644 --- a/.github/workflows/e2e.yml +++ b/.github/workflows/e2e.yml @@ -133,9 +133,6 @@ jobs: # Empty on Linux (full suite); "not semantic and not latency" on the # macOS/Windows legs so the OS-independent live-index checks run once. MARKERS: ${{ matrix.markers }} - # KEENABLE_E2E_WRITE_FEEDBACK is deliberately NOT set: the - # success-path feedback tests persist synthetic relevance data and - # are opt-in only (run them locally or on a staging tenant). run: | # The shell-installer test and daemon tests self-skip on Windows via # pytest skipif, so the same invocation works on every platform. diff --git a/CLAUDE.md b/CLAUDE.md index c4dddc4..77f759d 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -18,7 +18,7 @@ export KEENABLE_BIN=./target/release/keenable # default: `keenable` on PATH uv run --project tests/e2e pytest tests/e2e -v ``` -Markers: `-m "not latency"` / `-m "not semantic"` / `-m "not install"` to skip the slow/live-index/download groups. Success-path feedback tests persist synthetic relevance data in the live API and are skipped unless `KEENABLE_E2E_WRITE_FEEDBACK=1` — never enable that on a schedule. CI (`.github/workflows/e2e.yml`) installs the latest released binary via the installer script and runs the full suite nightly and on manual dispatch (with an optional version input). Requires the `KEENABLE_API_KEY` repo secret. +Markers: `-m "not latency"` / `-m "not semantic"` / `-m "not install"` to skip the slow/live-index/download groups. CI (`.github/workflows/e2e.yml`) installs the latest released binary via the installer script and runs the full suite nightly and on manual dispatch (with an optional version input). Requires the `KEENABLE_API_KEY` repo secret. The suite isolates each run by pointing `KEENABLE_HOME` at a temp dir (see "Config & path resolution" below). CI runs the matrix across **Linux, macOS, and Windows**. Unix installs the binary via the shell installer; Windows via the PowerShell installer (`keenable-cli-installer.ps1`). The Unix-only daemon tests and the shell-installer test self-skip on Windows (`os.name != "posix"`), so the same `pytest tests/e2e` invocation works on every platform. @@ -39,7 +39,7 @@ src/ ide.rs # Shared IDE definitions and config helpers configure_mcp.rs # Client detection, MCP configuration, interactive setup reset.rs # Remove Keenable MCP and restore defaults - search.rs # search, fetch, feedback commands + search.rs # search, fetch commands assets/ login_success.html # Styled OAuth callback success page login_failure.html # Styled OAuth callback failure page @@ -54,7 +54,7 @@ assets/ ### Output Format -All tool commands (`search`, `fetch`, `feedback`) output **YAML by default** (token-efficient for agents). +All tool commands (`search`, `fetch`) output **YAML by default** (token-efficient for agents). Use `-p` / `--pretty` flag for pretty-printed human-readable output. Use `--api-key ` (or the `KEENABLE_API_KEY` env var; flag wins) to override the stored key for one-off calls. Either override bypasses the daemon and goes direct. @@ -81,8 +81,8 @@ Supported keys and allowed values are defined in `KNOWN_KEYS` in `config_cmd.rs` ### Unauthenticated (Free Tier) Flow -All tool commands (`search`, `fetch`, `feedback`) work without login. When no API key is configured: -- Requests go to `/v1/{search,fetch,feedback}/public` endpoints (IP-based rate limits) +All tool commands (`search`, `fetch`) work without login. When no API key is configured: +- Requests go to `/v1/{search,fetch}/public` endpoints (IP-based rate limits) - The daemon starts with a bare HTTP client (no `X-API-Key` header) - Rate limit errors include a hint to run `keenable login` for higher limits diff --git a/src/commands/search.rs b/src/commands/search.rs index a430392..fef707b 100644 --- a/src/commands/search.rs +++ b/src/commands/search.rs @@ -101,11 +101,9 @@ async fn execute(req: &DaemonRequest, api_key_override: Option<&str>) -> Result< match daemon::daemon_request(req).await { Ok(resp) if resp.ok => return Ok(resp.data.unwrap_or(Value::Null)), Ok(resp) => return Err(daemon_response_to_api_error(resp)), - // If a non-idempotent request may already have reached the - // API, surface the failure instead of re-sending. - Err(daemon::DaemonError::AfterSend(e)) if !req.idempotent() => { - return Err(ApiError::request_failed(e)); - } + // Every command is a read, so a daemon failure is always safe to + // retry directly — regardless of whether it happened before or + // after the request may have reached the API. Err(_) => {} // Fall through to direct } } @@ -147,16 +145,6 @@ async fn execute(req: &DaemonRequest, api_key_override: Option<&str>) -> Result< .map_err(send_err)?; handle_response(resp).await } - "feedback" => { - let body = req.body.as_ref().ok_or_else(|| missing("body"))?; - let resp = client - .post(endpoint("/v1/feedback", authenticated)) - .json(body) - .send() - .await - .map_err(send_err)?; - handle_response(resp).await - } _ => Err(ApiError { status: 0, error: format!("Unknown command: {}", req.command), @@ -421,71 +409,6 @@ pub async fn fetch( } } -pub async fn feedback(query: &str, scores: &[String], human: bool, api_key: Option<&str>) { - // The API requires a non-empty comment per entry, so reject comment-less - // entries up front - let mut relevance: Vec = Vec::new(); - for entry in scores { - // URL may contain '=' (e.g. query params), so split from the right. - // Note this means the comment itself cannot contain '=' — its first - // '=' would be taken as the score separator. - let parts: Vec<&str> = entry.rsplitn(3, '=').collect(); - // rsplitn reverses: [comment, score, url] - if parts.len() < 3 || parts[0].is_empty() || parts[2].is_empty() { - ui::error(&format!( - "Invalid format: {}. Expected url=score=comment (comment is required).", - entry - )); - eprintln!(); - std::process::exit(1); - } - let (comment, score_str, url) = (parts[0], parts[1], parts[2]); - - let score: u32 = match score_str.parse() { - Ok(s) if s <= 5 => s, - _ => { - ui::error(&format!( - "Invalid score in '{}'. Must be 0-5. Expected url=score=comment — note the comment cannot contain '='.", - entry - )); - eprintln!(); - std::process::exit(1); - } - }; - relevance.push(json!({ - "url": url, - "score": score, - "comment": comment, - })); - } - - let body = json!({ - "query": query, - "relevance": relevance, - }); - - let req = DaemonRequest { - command: "feedback".to_string(), - body: Some(body), - ..Default::default() - }; - - let api_key = key_override(api_key); - let api_key = api_key.as_deref(); - match execute(&req, api_key).await { - Ok(data) => { - if human { - ui::header("keenable feedback"); - ui::success("Feedback submitted"); - eprintln!(); - return; - } - print_yaml(&json!({"status": "ok", "message": "Feedback submitted", "data": data})); - } - Err(e) => handle_api_error(e, human, api_key), - } -} - #[cfg(test)] mod tests { use super::*; diff --git a/src/daemon.rs b/src/daemon.rs index 6063511..ebb47bf 100644 --- a/src/daemon.rs +++ b/src/daemon.rs @@ -18,12 +18,6 @@ pub struct DaemonRequest { } impl DaemonRequest { - /// Non-idempotent commands must not be retried after a failure that may - /// have already delivered them. - pub fn idempotent(&self) -> bool { - self.command != "feedback" - } - /// Query params for GET /v1/fetch, shared by the daemon and the direct /// HTTP path so fetch params can't drift between them. None when `urls` /// is missing. @@ -53,17 +47,11 @@ pub struct DaemonResponse { pub error: Option, } -/// Whether a daemon failure happened before or after the request may have -/// been forwarded to the API — callers must not retry non-idempotent -/// commands after a possible delivery. +/// A daemon round-trip failed. Every command the daemon proxies is a read, so +/// callers always retry directly — whether the failure happened before or +/// after the request may have reached the API. pub enum DaemonError { - /// Failed before delivery — safe to retry directly. Unavailable, - /// The request may have reached the API before the failure. - // Only constructed by the Unix daemon implementation; the Windows stub - // never reaches a post-send failure. - #[cfg_attr(not(unix), allow(dead_code))] - AfterSend(String), } // ── Unix implementation (Unix sockets) ────────────────────────────────────── @@ -310,18 +298,6 @@ mod platform { ) .await } - "feedback" => { - let body = match &req.body { - Some(b) => b.clone(), - None => return err_response("Missing body"), - }; - send_api( - client - .post(endpoint("/v1/feedback", authenticated)) - .json(&body), - ) - .await - } "ping" => DaemonResponse { ok: true, data: None, @@ -423,17 +399,10 @@ mod platform { let mut lines = BufReader::new(reader).lines(); // Wait longer than the daemon's own 60s HTTP timeout, so a slow // upstream yields the daemon's structured error instead of a client - // timeout (which callers would treat as "maybe delivered"). + // timeout that falls back to a second, equally slow direct request. match tokio::time::timeout(Duration::from_secs(75), lines.next_line()).await { - Ok(Ok(Some(line))) => serde_json::from_str(&line) - .map_err(|e| DaemonError::AfterSend(format!("Invalid daemon response: {}", e))), - Ok(Ok(None)) => Err(DaemonError::AfterSend( - "Daemon closed connection".to_string(), - )), - Ok(Err(e)) => Err(DaemonError::AfterSend(format!("Read error: {}", e))), - Err(_) => Err(DaemonError::AfterSend( - "Daemon request timed out".to_string(), - )), + Ok(Ok(Some(line))) => serde_json::from_str(&line).map_err(|_| DaemonError::Unavailable), + _ => Err(DaemonError::Unavailable), } } } diff --git a/src/main.rs b/src/main.rs index aac754b..876bb16 100644 --- a/src/main.rs +++ b/src/main.rs @@ -202,26 +202,6 @@ enum Commands { api_key: Option, }, - /// Submit search relevance feedback (outputs YAML by default, use -p for pretty output) - #[command( - after_help = "Works without login (free tier). Log in for higher rate limits.\n\nScore format: url=score=comment (0=irrelevant, 5=perfect; comment is required)\n\nExamples:\n keenable feedback \"rust async\" \"https://tokio.rs=5=great overview\" \"https://unrelated.com=1=off topic\"" - )] - Feedback { - /// Original search query - query: String, - - /// URL=score=comment entries (score 0-5, comment required) - scores: Vec, - - /// Pretty-print output for humans instead of YAML - #[arg(short = 'p', long = "pretty")] - pretty: bool, - - /// API key (overrides stored key) - #[arg(long = "api-key")] - api_key: Option, - }, - /// Run the background daemon (internal, auto-started) #[command(hide = true)] Daemon, @@ -257,9 +237,7 @@ async fn main() { // Update check only for human-facing output: awaiting it would add up to // ~5s (on cache miss) to agent-facing YAML commands and the daemon. let wants_update_check = match &cli.command { - Commands::Search { pretty, .. } - | Commands::Fetch { pretty, .. } - | Commands::Feedback { pretty, .. } => *pretty, + Commands::Search { pretty, .. } | Commands::Fetch { pretty, .. } => *pretty, Commands::Daemon => false, _ => true, }; @@ -335,14 +313,6 @@ async fn main() { } => { commands::search::fetch(&url, live, prompt, pretty, api_key.as_deref()).await; } - Commands::Feedback { - query, - scores, - pretty, - api_key, - } => { - commands::search::feedback(&query, &scores, pretty, api_key.as_deref()).await; - } Commands::Daemon => { daemon::run_daemon().await; } diff --git a/tests/e2e/conftest.py b/tests/e2e/conftest.py index 6c05fe2..b79c60b 100644 --- a/tests/e2e/conftest.py +++ b/tests/e2e/conftest.py @@ -25,17 +25,9 @@ BIN = os.environ.get("KEENABLE_BIN", "keenable") API_KEY = os.environ.get("KEENABLE_API_KEY", "") -# Seeds basic_search; feedback tests must submit for this exact query +# Seeds basic_search SEARCH_QUERY = "rust async patterns" -# Successful feedback submissions persist synthetic relevance data in the live -# API (no test tenant / dry-run mode exists), so they are opt-in only and must -# never run on a schedule. -write_feedback = pytest.mark.skipif( - os.environ.get("KEENABLE_E2E_WRITE_FEEDBACK") != "1", - reason="persists synthetic feedback to the live API — opt in with KEENABLE_E2E_WRITE_FEEDBACK=1", -) - # Tests that rely on Unix-only behavior — the Unix-domain-socket daemon and the # POSIX shell installer. On Windows the CLI stubs the daemon out and ships a # PowerShell installer instead, so these self-skip there. @@ -146,7 +138,7 @@ def mcp(tmp_path) -> Runner: @pytest.fixture(scope="session") def basic_search(kn) -> dict: - """T-01 search output, reused by schema/count/feedback tests.""" + """T-01 search output, reused by schema/count tests.""" res = kn("search", SEARCH_QUERY) assert res.code == 0, res.err return res.yaml() diff --git a/tests/e2e/test_feedback.py b/tests/e2e/test_feedback.py deleted file mode 100644 index 32bb369..0000000 --- a/tests/e2e/test_feedback.py +++ /dev/null @@ -1,62 +0,0 @@ -"""T-30..T-36 — feedback. Valid feedback requires a recent search for the -same query with the same key, so tests depend on the basic_search fixture. - -Success-path tests persist real feedback and carry the write_feedback opt-in -gate; the error-path tests are rejected server- or client-side and are safe. -""" - -from conftest import SEARCH_QUERY as QUERY # must match basic_search's query -from conftest import write_feedback - - -@write_feedback -def test_valid_feedback(kn, basic_search): - res = kn("feedback", QUERY, "https://tokio.rs=5=synthetic e2e feedback, ignore") - assert res.code == 0, res.out + res.err - data = res.yaml() - assert data["status"] == "ok" - - -def test_score_without_comment_rejected(kn): - res = kn("feedback", QUERY, "https://tokio.rs=4") - assert res.code == 1 - # ui::error word-wraps to terminal width, so assert wrap-safe fragments. - assert "Invalid format: https://tokio.rs=4" in res.err - assert "url=score=comment" in res.err - - -@write_feedback -def test_multiple_scores(kn, basic_search): - res = kn("feedback", QUERY, - "https://tokio.rs=5=synthetic e2e feedback, ignore", - "https://example.com=1=synthetic e2e feedback, ignore") - assert res.code == 0, res.out + res.err - assert res.yaml()["status"] == "ok" - - -def test_feedback_for_unsearched_query(kn): - res = kn("feedback", "never searched this xyz qqq", "https://x.com=5=test comment") - assert res.code == 1 - data = res.yaml() - assert data["error"] == "Bad request" - assert "does not match any recent search" in data["message"] - - -def test_out_of_range_score(kn): - res = kn("feedback", QUERY, "https://tokio.rs=9=too good") - assert res.code == 1 - assert "Invalid score in 'https://tokio.rs=9=too good'. Must be 0-5." in res.err - - -def test_no_scores(kn, basic_search): - res = kn("feedback", QUERY) - assert res.code == 1 - data = res.yaml() - assert data["error"] == "Invalid parameter" - assert "between 1 and 50 entries" in data["message"] - - -def test_malformed_score_entry(kn): - res = kn("feedback", QUERY, "no-equals-sign") - assert res.code == 1 - assert "Invalid format: no-equals-sign" in res.err diff --git a/tests/e2e/test_global.py b/tests/e2e/test_global.py index 22c9d5f..50f1661 100644 --- a/tests/e2e/test_global.py +++ b/tests/e2e/test_global.py @@ -12,7 +12,7 @@ def test_version(kn): def test_help_lists_subcommands(kn): res = kn("--help", key=False) assert res.code == 0 - for sub in ("login", "logout", "configure-mcp", "reset", "config", "search", "fetch", "feedback"): + for sub in ("login", "logout", "configure-mcp", "reset", "config", "search", "fetch"): assert sub in res.out, f"--help missing subcommand {sub}" diff --git a/tests/e2e/test_login_flow.py b/tests/e2e/test_login_flow.py index b3bfc1d..941984f 100644 --- a/tests/e2e/test_login_flow.py +++ b/tests/e2e/test_login_flow.py @@ -2,7 +2,7 @@ Everything else in the suite passes --api-key, which skips the daemon and goes direct HTTP. These tests run `keenable login --api-key` once, then exercise -search/fetch/feedback WITHOUT the flag, so the stored key is resolved from +search/fetch WITHOUT the flag, so the stored key is resolved from config and requests go through the background daemon. Logout flips the same commands to the public (free-tier) endpoints. """ @@ -21,7 +21,6 @@ Runner, requires_posix, search_results, - write_feedback, ) @@ -61,14 +60,6 @@ def test_login_then_fetch_via_daemon(logged_in): assert res.yaml()["title"] == "Example Domain" -@write_feedback -def test_login_then_feedback_via_daemon(logged_in): - assert logged_in("search", QUERY, key=False).code == 0 - res = logged_in("feedback", QUERY, "https://tokio.rs=5=synthetic e2e feedback, ignore", key=False) - assert res.code == 0, res.out + res.err - assert res.yaml()["status"] == "ok" - - def test_logout_clears_key_and_falls_back_to_public(logged_in): res = logged_in("logout", key=False) assert res.code == 0