Skip to content

fix(telegram): emit chat_type metadata for group-safe prompt behavior - #2513

Merged
ilblackdragon merged 1 commit into
nearai:stagingfrom
rozgo:fix/2482-telegram-chat-type-metadata
Apr 18, 2026
Merged

ilblackdragon merged 1 commit into
nearai:stagingfrom
rozgo:fix/2482-telegram-chat-type-metadata

Conversation

@rozgo

@rozgo rozgo commented Apr 16, 2026 •

Copy link
Copy Markdown
Contributor

Summary

  • emit metadata.chat_type for inbound Telegram messages using Telegram's source chat type
  • keep TelegramMessageMetadata backward-compatible by making chat_type optional on deserialize
  • add Telegram regression coverage for legacy metadata parsing and group message emission

Linked Issue

Fixes #2482

Change Type

  • Bug fix

Review Track

  • B

Validation

  • cargo fmt --all -- --check
  • cargo clippy --all --benches --tests --examples --all-features -- -D warnings
  • cargo build
  • cargo test test_telegram_message_metadata_deserializes_without_chat_type (in channels-src/telegram)
  • cargo test --features integration --test telegram_auth_integration test_group_message_emits_chat_type_metadata -- --nocapture
  • cargo test

cargo test currently fails in an unrelated upstream e2e assertion:

tests::tool_info_clarifies_message_and_channel_setup_roles in tests/e2e_builtin_tool_coverage.rs

This failure is outside the Telegram diff.

Impact

  • Security Impact: none
  • Database Impact: none
  • Blast Radius: Telegram metadata emission and downstream group/private prompt detection
  • Rollback Plan: revert this commit

@github-actions github-actions Bot added size: S 10-49 changed lines risk: low Changes to docs, tests, or low-risk modules contributor: new First-time contributor labels Apr 16, 2026

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request introduces a chat_type field to the TelegramMessageMetadata struct to facilitate downstream detection of group and private chats. The changes include updating the message handling logic to populate this field, adding a unit test for backward compatibility during deserialization, and adding an integration test to verify metadata emission. A review comment suggests improving type safety by using a dedicated enum for the chat_type field instead of a raw String.

Comment on lines +305 to +307
/// Telegram chat type for downstream group/private detection.
#[serde(default, skip_serializing_if = "Option::is_none")]
chat_type: Option<String>,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

For better type safety and to make the code more robust, consider using an enum for chat_type instead of a String. The possible values are well-defined (private, group, supergroup, channel).

Using an enum aligns with the 'Parse, Don't Validate' principle, ensuring type safety at the boundary where this data is processed. This would make downstream logic that consumes this metadata less prone to errors from string comparisons.

You could define an enum like this:
#[derive(Debug, Serialize, Deserialize, Clone, PartialEq, Eq)]
#[serde(rename_all = "lowercase")]
pub enum TelegramChatType {
Private,
Group,
Supergroup,
Channel,
}

Then, you would change this field's type to Option<TelegramChatType>. This would also require updating handle_message to parse the string from message.chat.chat_type into this new enum.

References
  1. For inputs that require validation (like timezone strings), prefer creating a newtype wrapper that validates on construction. This 'Parse, Don't Validate' approach ensures type safety and centralizes validation at system boundaries.

@henrypark133 henrypark133 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Review: Telegram metadata fix looks correct

I verified the chat_type emission path and the added compatibility coverage. I did not find a blocker in the changed code.

Positives:

  • Inbound Telegram messages now persist the source chat type in metadata without changing the existing is_private behavior.
  • Deserialization stays backward-compatible because chat_type is optional on read.
  • The integration test hits the real webhook path and asserts the emitted metadata, which is the right caller-level coverage for this fix.

Residual risk:

  • This is based on code inspection plus the targeted tests in the diff; I did not run the Telegram integration suite from this review pass.

@rozgo
rozgo force-pushed the fix/2482-telegram-chat-type-metadata branch from 87374b5 to f382ab9 Compare April 17, 2026 23:46

rozgo commented Apr 17, 2026

Copy link
Copy Markdown
Contributor Author

Rebased this branch onto current staging and force-pushed with --force-with-lease to pick up upstream CI fixes; no functional changes to the Telegram patch. Local revalidation on the rebased branch: cargo fmt, cargo clippy, cargo build, the Telegram metadata compatibility unit test, and the Telegram group-message integration test all pass. Full cargo test currently fails in an unrelated upstream assertion in tests/e2e_builtin_tool_coverage.rs.

@ilblackdragon
ilblackdragon merged commit dcca218 into nearai:staging Apr 18, 2026
15 checks passed
@ilblackdragon ilblackdragon mentioned this pull request Apr 18, 2026
5 of 7 tasks
ilblackdragon added a commit that referenced this pull request Apr 20, 2026
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>
@henrypark133 henrypark133 mentioned this pull request Apr 21, 2026
ilblackdragon added a commit that referenced this pull request Apr 21, 2026
* 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>
theredspoon pushed a commit to theredspoon/ironclaw that referenced this pull request Jun 21, 2026
theredspoon pushed a commit to theredspoon/ironclaw that referenced this pull request Jun 21, 2026
* 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 nearai#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 nearai#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 nearai#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 (nearai#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 nearai#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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

contributor: new First-time contributor risk: low Changes to docs, tests, or low-risk modules size: S 10-49 changed lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[QA] Telegram group messages omit chat_type metadata and skip group-chat prompt behavior

3 participants