mirror of
https://github.com/nearai/ironclaw.git
synced 2026-09-03 08:06:01 +08:00
8fffa8797c
* fix(tests): close the staging test backlog — rust suite green, e2e 14→4 A pass over staging turned up 12 rust test failures and 14 playwright e2e failures + 1 fixture error. Most were wiring/invariant drift or stale test expectations around engine v2. This patch cleans up the ones with clear root causes. Rust (12 → 0): - `tools::builtin::skill_tools` (8 tests): ripped out hand-rolled ZIP byte blobs that were missing the EOCD record since the extractor switched to `zip::ZipArchive::new` in #2385. Tests now build through `zip::ZipWriter`, matching the production path. Drops the obsolete nested-path assertion whose assumption conflicts with intentional GitHub-archive root stripping. - `extensions::manager::test_telegram_token_colon_preserved_in_validation_url`: `src/pairing/approval.rs::propagate_approval_restores_runtime_state_when_on_start_fails` was mutating the `IRONCLAW_TEST_TELEGRAM_API_BASE_URL` runtime-env overlay without holding `ENV_MUTEX`. Now acquires `lock_env()` so concurrent readers see a stable value. - `bridge::router::handle_with_engine_persists_attachment_files_and_indexes_them`: two distinct `ENGINE_STATE_TEST_LOCK` statics (one in `test_support`, one in the sibling `tests` module) meant cross-module tests raced on the shared `ENGINE_STATE` `OnceLock`. Replaced the private duplicate with `use super::test_support::ENGINE_STATE_TEST_LOCK`. - `e2e_attachments::engine_v2_channel_attachments_persist_for_telegram_and_whatsapp`: attachment persistence resolves paths through the cached `bootstrap::ironclaw_base_dir()`, not the test's tempdir CWD. Added `bridge::override_engine_project_root_for_test` and wired the test to use it. - `telegram_auth_integration::test_group_message_emits_chat_type_metadata`: local fix — rebuild `channels-src/telegram` so the WASM picks up the April-17 `chat_type` emit from #2513. CI rebuilds the module per run, so no binary committed here. Playwright (14 failed + 1 error → 4 failed + 1 error): - `test_chat.py::test_gateway_attachment_flow_renders_thread_and_reaches_llm` and the unextractable variant: a legacy change listener on `#image-file-input` fired before the unified `handleAttachmentFiles` path, cleared `e.target.value`, and left the FileList empty by the time the unified handler ran. Removed the duplicate wiring in `crates/ironclaw_gateway/static/js/surfaces/chat.js`. - `test_chat.py::test_slash_autocomplete_shows_commands_and_skills`: `SLASH_COMMANDS` never merged installed skills. Added `refreshSlashSkillEntries()` that fetches `/api/skills` on menu open and re-runs the filter once the skills land. - `test_pending_user_messages.py::test_pending_message_survives_sse_reconnect`: the SSE open handler only reloads history when `disconnectMs > SSE_RELOAD_THRESHOLD_MS`; the test's instant reconnect skipped that. Ages `_sseDisconnectedAt` past threshold. - `test_pending_user_messages.py::test_welcome_card_hidden_when_pending`: `_create_new_thread` returned `currentThreadId` before the new-thread API round-trip set it, so callers got the pre-click id and keyed `_pendingUserMessages` on the wrong thread. Now waits for the id to change. - `TestV2EngineSkillInstallFlow` (7 → 2 failures): - Skill card template didn't render `usage_hint`, `has_requirements`, `has_scripts`, or `install_source_url`. Extended `renderSkillCard` in `surfaces/skills.js`. - The deny message `"Do not execute it; choose an alternative approach"` accidentally matched `user_signals_execution_intent`'s EXEC_PHRASES ("execute it"), re-arming `require_action_attempt` on resume and nudging the LLM into another tool call. Rephrased to `"Do not retry; choose a different approach"` in `src/bridge/router.rs`. Partial progress (still failing, needs deeper engine-v2 work): - `test_v2_engine_oauth_google::test_oauth_token_refresh_on_expiry`: added an `oauth:` block to the test's `google_drive` skill (which registers a refresh config via `credential_spec_to_oauth_refresh`) and aligned `GOOGLE_OAUTH_CLIENT_ID` with the mock proxy's expected `hosted-google-client-id`. Thread still hits the auth gate instead of refreshing — the pre-flight path isn't reaching `oauth_refresh_for_secret("google_drive_token")`; needs instrumentation on the engine-v2 gate pipeline. Net: rust suite green, playwright 4 failures left (2 skill-install approval-flow edge cases, 1 OAuth refresh, 1 REPL auth that flakes only under full-suite load) + 1 restart-fixture health-check timeout that flakes under 20-min suite pressure. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(tests): close the final 4 e2e failures + add copy-button coverage Follow-up to the earlier staging test pass. Drives the remaining playwright failures to green and adds the missing test for the per-message Copy button. New coverage: - `test_chat.py::test_message_copy_button_writes_raw_text`: clicking the per-message Copy button writes the raw text (user turn) or the raw markdown (assistant, via `data-raw`) to navigator.clipboard and flashes the button label to "Copied!" then back to "Copy". The existing `test_copy_from_chat_forces_plain_text` only covered the Cmd+C selection handler, so a regression to the button path was invisible. Fixes: - `TestV2EngineSkillInstallFlow::test_implicit_skill_activation_works_immediately_after_install`: the pika skill manifest uses the legacy `metadata.openclaw.requires` shape without a top-level `activation:` block, so `score_skill` scored 0 for every prompt and the skill never activated unless the user typed `/pikastream-video-meeting`. "Please use pikastream-video-meeting to prepare this call" should activate just like the slash form. `score_skill` now treats the skill name (and the hyphen/underscore-normalized form) as an implicit keyword, gated at ≥4 chars so short generic names don't false-match. `test_installed_skill_does_not_overfire_on_unrelated_prompt` still passes — a grocery-list prompt doesn't accidentally trigger pika. - `TestV2EngineSkillInstallFlow::test_duplicate_install_is_idempotent_and_keeps_single_card`: the test was waiting for an approval card on the second install, but `SkillInstallTool::requires_approval` short-circuits to `ApprovalRequirement::Never` when the skill is already loaded — asking the user to approve a guaranteed no-op is pure friction, and the test was asserting against that intentional behavior. Rewrote the test to skip the approval step and assert on the terminal message's idempotent "already installed / no install needed" wording, which matches the actual production output. - Mock LLM: the pattern branch in `match_tool_call` was re-emitting a matching tool call on every LLM round because "last user content" doesn't change across turns, so the engine looped until it hit the multi-result summary path. Added a guard that falls through to the text-response path when the matching tool_name is already present in `recent_tool_results` — mirroring real LLM behavior. - `test_v2_engine_oauth_google::test_oauth_token_refresh_on_expiry`: two compounding issues blocked the refresh path. (1) The mock `/oauth/refresh` handler validates `client_id == "hosted-google- client-id"`, but the fixture env set `test-google-client-id`. (2) Proxy URL points at `http://127.0.0.1:<port>` (the mock LLM) and the production SSRF guard blocks loopback by default; mock E2E tests opt in via `IRONCLAW_OAUTH_PROXY_ALLOW_LOOPBACK=1`. Also added an `oauth:` block to the test's `google_drive` skill so `credential_spec_to_oauth_refresh` registers a refresh config under `google_drive_token`. Finally, the refresh path needs a stored refresh token — paste-based auth (the earlier tests' fallback when no google-drive WASM binary is available) only persists the access token, so the test now skips in that configuration rather than asserting on a refresh that can't happen, matching the pattern already used by `test_oauth_redirect_flow`. Remaining after this PR: `test_repl_http_auth_prompt_accepts_token_and_retries` passes in isolation but flakes under full-suite load (the PTY REPL sibling test is already `@pytest.mark.skip` for the same reason); and `test_always_approve_survives_restart` which times out the `/api/health` probe under full-suite pressure. Both are PTY / fixture-startup concurrency issues, not product regressions. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(tests): close the last 2 e2e failures — full suite green (401 passed, 0 failed) Root-causes the two tests left open after the previous commit. Both were real bugs/config drift masquerading as flakiness. - `test_repl_http_auth_prompt_accepts_token_and_retries`: `CLI_MODE` defaults to `tui` (the ratatui full-screen UI), which reads stdin keystroke-by-keystroke and renders into a framebuffer. The PTY-driven tests in this file send whole lines via `os.write(master_fd, b"prompt\n")` and match for specific text in the raw stream — under the default TUI that line-based send never reaches the agent, so the auth card never fires and `_read_repl_until` times out with only cursor-position escape sequences captured. Pinning `CLI_MODE=repl` on the fixture routes the test back onto the plain line-based REPL surface it's written against. Confirmed passing 5/5 in isolation and under full-suite load. - `test_always_approve_survives_restart`: the fixture's ironclaw subprocess was dying at startup with `Channel webhook_server failed to start: Failed to bind to 127.0.0.1:8080: Address already in use (os error 98)` — the fixture picked a free `GATEWAY_PORT` but left `HTTP_HOST`/ `HTTP_PORT` unset, so the HTTP channel tried to claim the default port 8080 and collided with every other e2e server (and anything else on 8080). Every `/api/health` probe was hitting a dead process, which showed up as a 60 s timeout instead of a bind error because the subprocess's stderr was never drained — a full 64 KiB pipe buffer made the child block on its next write before it could even log the bind failure. Fix: - allocate a second free TCP port for `HTTP_PORT` (mirrors the sibling `v2_approval_server` fixture); - wire `stdout`/`stderr` through background drain tasks so `RUST_LOG=ironclaw=debug` output can't back-pressure the child into a startup hang; - surface the last 32 KiB of stderr in the timeout error so future regressions (panic, bind conflict) show up in the failure message instead of being silently swallowed. Full-suite e2e: 401 passed, 8 skipped, 0 failed, 0 errored (17:31). Rust unit + integration tests still green, clippy clean, fmt clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(tests): address PR #2744 review + reconcile with staging ## Review feedback - **Slash-skill cache spam (3× — Gemini + 2× Copilot):** the previous `refreshSlashSkillEntries()` re-fetched `/api/skills` on every keystroke in `filterSlashCommands`; the in-flight guard only suppressed concurrent duplicates. Added a 30 s TTL and an `invalidateSlashSkillCache()` hook that the install/remove flows in `surfaces/skills.js` call so the menu picks up install/remove changes immediately instead of waiting for the TTL. - **Wrong module path in comment (Copilot):** `src/bridge/router.rs` comment referenced `llm::reasoning::user_signals_execution_intent` but `reasoning` isn't `pub` — the helper is re-exported as `crate::llm::user_signals_execution_intent`. Updated the comment to use the canonical path and cross-reference the defining file. - **Misleading `#[tokio::test]` justification (Copilot):** prior comment said "single-threaded tokio and cannot deadlock" without pinning the runtime flavor. `#[tokio::test]` *does* default to the current-thread runtime in this crate, but spelling it out is safer against future defaults drifting. Pinned `#[tokio::test(flavor = "current_thread")]` explicitly and reworded the comment to name the runtime kind. - **Drain tasks cancelled but not awaited (Copilot):** the restart fixture in `test_v2_engine_approval_flow.py` cancelled the stdout/stderr drainers on `stop()` without awaiting them, causing "Task was destroyed but it is pending!" warnings and, on stop→start cycles, zombie readers. Now cancels *and* `asyncio.gather (..., return_exceptions=True)` awaits them. ## Merge reconciliation with `origin/staging` Staging merge introduced: - A strict MIME allowlist on `/api/chat/send` attachments (#2332). `test_gateway_attachment_unextractable_file_uses_placeholder` previously relied on `application/octet-stream` reaching `document_extraction` and triggering the "[Failed to extract …]" placeholder; the new gateway-side allowlist rejects that MIME outright at the HTTP layer, so the test never exercised the fallback path. Updated the test to upload a corrupt PDF (`%PDF-1.4` magic + garbage body) which passes MIME + header checks but fails extraction — the exact scenario the placeholder was designed for. - A conflict in `src/pairing/approval.rs` where staging added `#[ignore]` to the propagate-approval test (needs a pre-built telegram WASM binary) and this branch added `#[allow(clippy::await_holding_lock)]`. Merged both, plus pinned the explicit `current_thread` runtime flavor per review. ## Pre-existing failures left alone `test_portfolio.py::test_portfolio_chat_keyword_triggers_skill` and `test_portfolio_wallet_address_triggers_skill` both fail identically against plain `origin/staging` (verified via `git stash` + checkout of the staging versions of the test file and `crates/ironclaw_engine/orchestrator/default.py`). Root cause is unrelated to this PR — appears to be the mock LLM's portfolio response text tripping the engine's tool-intent nudge path before reaching the canned response the test asserts on. Out of scope here. ## Verification - `cargo fmt` - `cargo clippy --all --benches --tests --examples --all-features` — zero warnings - `cargo test --lib` — 5329 passed, 7 ignored, 0 failed - `pytest scenarios/test_chat.py scenarios/test_v2_engine_approval_flow.py scenarios/test_v2_engine_auth_flow.py::TestV2EngineSkillInstallFlow scenarios/test_v2_auth_oauth_matrix.py scenarios/test_pending_user_messages.py` — **58 passed, 1 skipped, 0 failed** Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(fmt): collapse override_engine_project_root call onto single line rustfmt on staging collapses this call; my earlier `cargo fmt` ran before the `project_root.clone()` edit landed so the local check missed it. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Address PR #2744 review: startup-timeout leak + attachment test isolation 1. `test_v2_engine_approval_flow.py` — `start()` re-raised `TimeoutError` from `wait_for_ready` without tearing the subprocess down. Because `await start()` runs before the fixture's `try/finally`, a startup timeout would leak the child process and its bound ports into the rest of the test run. Snapshot the stderr tail before teardown, `await stop()` (which kills the proc and cancels/awaits the drain tasks), then re-raise with the captured tail. 2. `tests/e2e_attachments.rs` — the `engine_v2_project_root()` helper derived from `bootstrap::ironclaw_base_dir()` is a process-global `LazyLock` that resolves to `$HOME/.ironclaw` on dev machines and CI runners. Passing its parent as the engine's project_root meant this test was writing real attachment files into `~/.ironclaw/attachments` every time it ran. Allocate a per-test `tempfile::TempDir` instead and point `override_engine_project_root_for_test` at it — now writes are fully contained. The `engine_v2_attachment_root_lock` mutex stays (still required to serialize mutations of the process-global engine state across concurrent tests). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>