Fixes build, adds missing sse event and correct command - #11
Merged
Merged
Conversation
elliotBraem
marked this pull request as ready for review
February 9, 2026 23:13
ilblackdragon
approved these changes
Feb 10, 2026
Merged
serrrfirat
pushed a commit
to serrrfirat/ironclaw
that referenced
this pull request
Feb 16, 2026
* add missing type * prune * readme * minor * update to .ironclaw
5 tasks
ilblackdragon
added a commit
that referenced
this pull request
Feb 19, 2026
- Use manifest.name (not crate_name) for installed filenames so discovery, auth, and CLI commands all agree on the stem (#1) - Add AlreadyInstalled error variant instead of misleading ExtensionNotFound (#2) - Add DownloadFailed error variant with URL context instead of stuffing URLs into PathBuf (#3) - Validate HTTP status with error_for_status() before reading response bytes in artifact downloads (#4) - Switch build_wasm_component to tokio::process::Command with status() so build output streams to the terminal (#6) - Find WASM artifact by crate_name specifically instead of picking the first .wasm file in the release directory (#7) - Add is_file() guard in catalog loader to skip directories (#8) - Detect ambiguous bare-name lookups when both tools/<name> and channels/<name> exist, with get_strict() returning an error (#9) - Fix wizard step_extensions to check tool.name for installed detection, consistent with the new naming (#11, #12) - Fix redundant closures and map_or clippy warnings in changed files Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
ilblackdragon
added a commit
that referenced
this pull request
Feb 20, 2026
- Use manifest.name (not crate_name) for installed filenames so discovery, auth, and CLI commands all agree on the stem (#1) - Add AlreadyInstalled error variant instead of misleading ExtensionNotFound (#2) - Add DownloadFailed error variant with URL context instead of stuffing URLs into PathBuf (#3) - Validate HTTP status with error_for_status() before reading response bytes in artifact downloads (#4) - Switch build_wasm_component to tokio::process::Command with status() so build output streams to the terminal (#6) - Find WASM artifact by crate_name specifically instead of picking the first .wasm file in the release directory (#7) - Add is_file() guard in catalog loader to skip directories (#8) - Detect ambiguous bare-name lookups when both tools/<name> and channels/<name> exist, with get_strict() returning an error (#9) - Fix wizard step_extensions to check tool.name for installed detection, consistent with the new naming (#11, #12) - Fix redundant closures and map_or clippy warnings in changed files Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
ilblackdragon
added a commit
that referenced
this pull request
Feb 20, 2026
…tion (#238) * feat: add extension registry with metadata catalog, CLI, and onboarding integration Adds a central registry that catalogs all 14 available extensions (10 tools, 4 channels) with their capabilities, auth requirements, and artifact references. The onboarding wizard now shows installable channels from the registry and offers tool installation as a new Step 7. - registry/ folder with per-extension JSON manifests and bundle definitions - src/registry/ module: manifest structs, catalog loader, installer - `ironclaw registry list|info|install|install-defaults` CLI commands - Setup wizard enhanced: channels from registry, new extensions step (8 steps) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(setup): resolve workspace errors for tool crates and channels-only onboarding Tool crates in tools-src/ and channels-src/ failed `cargo metadata` during onboard install because Cargo resolved them as part of the root workspace. Add `[workspace]` table to each standalone crate and extend the root `workspace.exclude` list so they build independently. Channels-only mode (`onboard --channels-only`) failed with "Secrets not configured" and "No database connection" because it skipped database and security setup. Add `reconnect_existing_db()` to establish the DB connection and load saved settings before running channel configuration. Also improve the tunnel "already configured" display to show full provider details (domain, mode, command) instead of just the provider name. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(registry): address PR review feedback on installer and catalog - Use manifest.name (not crate_name) for installed filenames so discovery, auth, and CLI commands all agree on the stem (#1) - Add AlreadyInstalled error variant instead of misleading ExtensionNotFound (#2) - Add DownloadFailed error variant with URL context instead of stuffing URLs into PathBuf (#3) - Validate HTTP status with error_for_status() before reading response bytes in artifact downloads (#4) - Switch build_wasm_component to tokio::process::Command with status() so build output streams to the terminal (#6) - Find WASM artifact by crate_name specifically instead of picking the first .wasm file in the release directory (#7) - Add is_file() guard in catalog loader to skip directories (#8) - Detect ambiguous bare-name lookups when both tools/<name> and channels/<name> exist, with get_strict() returning an error (#9) - Fix wizard step_extensions to check tool.name for installed detection, consistent with the new naming (#11, #12) - Fix redundant closures and map_or clippy warnings in changed files Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(setup): restore DB connection fields after settings reload reconnect_postgres() and reconnect_libsql() called Settings::from_db_map() which overwrote database_url / libsql_path / libsql_url set from env vars. Also use get_strict() in cmd_info to surface ambiguous bare-name errors. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * style: fix clippy collapsible_if and print_literal warnings Collapse nested if-let chains and inline string literals in format macros to satisfy CI clippy lint checks (deny warnings). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(registry): prefer artifacts for install-defaults and improve dir lookup - InstallDefaults now defaults to downloading pre-built artifacts (matching `registry install` behavior), with --build flag for source builds. - find_registry_dir() walks up 3 ancestor levels from the exe and adds a CARGO_MANIFEST_DIR fallback, matching load_registry_catalog() logic. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
jaswinder6991
pushed a commit
to jaswinder6991/ironclaw
that referenced
this pull request
Feb 26, 2026
…tion (nearai#238) * feat: add extension registry with metadata catalog, CLI, and onboarding integration Adds a central registry that catalogs all 14 available extensions (10 tools, 4 channels) with their capabilities, auth requirements, and artifact references. The onboarding wizard now shows installable channels from the registry and offers tool installation as a new Step 7. - registry/ folder with per-extension JSON manifests and bundle definitions - src/registry/ module: manifest structs, catalog loader, installer - `ironclaw registry list|info|install|install-defaults` CLI commands - Setup wizard enhanced: channels from registry, new extensions step (8 steps) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(setup): resolve workspace errors for tool crates and channels-only onboarding Tool crates in tools-src/ and channels-src/ failed `cargo metadata` during onboard install because Cargo resolved them as part of the root workspace. Add `[workspace]` table to each standalone crate and extend the root `workspace.exclude` list so they build independently. Channels-only mode (`onboard --channels-only`) failed with "Secrets not configured" and "No database connection" because it skipped database and security setup. Add `reconnect_existing_db()` to establish the DB connection and load saved settings before running channel configuration. Also improve the tunnel "already configured" display to show full provider details (domain, mode, command) instead of just the provider name. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(registry): address PR review feedback on installer and catalog - Use manifest.name (not crate_name) for installed filenames so discovery, auth, and CLI commands all agree on the stem (nearai#1) - Add AlreadyInstalled error variant instead of misleading ExtensionNotFound (nearai#2) - Add DownloadFailed error variant with URL context instead of stuffing URLs into PathBuf (nearai#3) - Validate HTTP status with error_for_status() before reading response bytes in artifact downloads (nearai#4) - Switch build_wasm_component to tokio::process::Command with status() so build output streams to the terminal (nearai#6) - Find WASM artifact by crate_name specifically instead of picking the first .wasm file in the release directory (nearai#7) - Add is_file() guard in catalog loader to skip directories (nearai#8) - Detect ambiguous bare-name lookups when both tools/<name> and channels/<name> exist, with get_strict() returning an error (nearai#9) - Fix wizard step_extensions to check tool.name for installed detection, consistent with the new naming (nearai#11, nearai#12) - Fix redundant closures and map_or clippy warnings in changed files Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(setup): restore DB connection fields after settings reload reconnect_postgres() and reconnect_libsql() called Settings::from_db_map() which overwrote database_url / libsql_path / libsql_url set from env vars. Also use get_strict() in cmd_info to surface ambiguous bare-name errors. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * style: fix clippy collapsible_if and print_literal warnings Collapse nested if-let chains and inline string literals in format macros to satisfy CI clippy lint checks (deny warnings). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(registry): prefer artifacts for install-defaults and improve dir lookup - InstallDefaults now defaults to downloading pre-built artifacts (matching `registry install` behavior), with --build flag for source builds. - find_registry_dir() walks up 3 ancestor levels from the exe and adds a CARGO_MANIFEST_DIR fallback, matching load_registry_catalog() logic. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
5 tasks
ilblackdragon
added a commit
that referenced
this pull request
Mar 7, 2026
…lity Security fixes: - Remove SSRF-prone download() from DocumentExtractionMiddleware (#13) - Sanitize filenames in workspace path to prevent directory traversal (#11) - Pre-check file size before reading in WASM wrapper to prevent OOM (#2) - Percent-encode file_id in Telegram source URLs (#7) Correctness fixes: - Clear image_content_parts on turn end to prevent memory leak (#1) - Find first *successful* transcription instead of first overall (#3) - Enforce data.len() size limit in document extraction (#10) - Use UTF-8 safe truncation with char_indices() (#12) Robustness & code quality: - Add 120s timeout to OpenAI Whisper HTTP client (#5) - Trim trailing slash from Whisper base_url (#6) - Allow ~/.ironclaw/ paths in WASM wrapper (#8) - Return error from on_broadcast in Slack/Discord/WhatsApp (#9) - Fix doc comment in HTTP tool (#4) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
ilblackdragon
added a commit
that referenced
this pull request
Mar 7, 2026
* feat: add inbound attachment support to WASM channel system Add attachment record to WIT interface and implement inbound media parsing across all four channel implementations (Telegram, Slack, WhatsApp, Discord). Attachments flow from WASM channels through EmittedMessage to IncomingMessage with validation (size limits, MIME allowlist, count caps) at the host boundary. - Add `attachment` record to `emitted-message` in wit/channel.wit - Add `IncomingAttachment` struct to channel.rs and re-export - Add host-side validation (20MB total, 10 max, MIME allowlist) - Telegram: parse photo, document, audio, video, voice, sticker - Slack: parse file attachments with url_private - WhatsApp: parse image, audio, video, document with captions - Discord: backward-compatible empty attachments - Update FEATURE_PARITY.md section 7 - Add fixture-based tests per channel and host integration tests [skip-regression-check] Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * feat: integrate outbound attachment support and reconcile WIT types (#409) Reconcile PR #409's outbound attachment work with our inbound attachment support into a unified design: WIT type split: - `inbound-attachment` in channel-host: metadata-only (id, mime_type, filename, size_bytes, source_url, storage_key, extracted_text) - `attachment` in channel: raw bytes (filename, mime_type, data) on agent-response for outbound sending Outbound features (from PR #409): - `on-broadcast` WIT export for proactive messages without prior inbound - Telegram: multipart sendPhoto/sendDocument with auto photo→document fallback for files >10MB - wrapper.rs: `call_on_broadcast`, `read_attachments` from disk, attachment params threaded through `call_on_respond` - HTTP tool: `save_to` param for binary downloads to /tmp/ (50MB limit, path traversal protection, SSRF-safe redirect following) - Message tool: allow /tmp/ paths for attachments alongside base_dir - Credential env var fallback in inject_channel_credentials Channel updates: - All 4 channels implement on_broadcast (Telegram full, others stub) - Telegram: polling_enabled config, adjusted poll timeout - Inbound attachment types renamed to InboundAttachment in all channels Tests: 1965 passing (9 new), 0 clippy warnings [skip-regression-check] Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * feat: add audio transcription pipeline and extensible WIT attachment design Add host-side transcription middleware (OpenAI Whisper) that detects audio attachments with inline data on incoming messages and transcribes them automatically. Refactor WIT inbound-attachment to use extras-json and a store-attachment-data host function instead of typed fields, so future attachment properties (dimensions, codec, etc.) don't require WIT changes that invalidate all channel plugins. - Add src/transcription/ module: TranscriptionProvider trait, TranscriptionMiddleware, AudioFormat enum, OpenAI Whisper provider - Add src/config/transcription.rs: TRANSCRIPTION_ENABLED/MODEL/BASE_URL - Wire middleware into agent message loop via AgentDeps - WIT: replace data + duration-secs with extras-json + store-attachment-data - Host: parse extras-json for well-known keys, merge stored binary data - Telegram: download voice files via store-attachment-data, add duration to extras-json, add /file/bot to HTTP allowlist, voice-only placeholder - Add reqwest multipart feature for Whisper API uploads - 5 regression tests for transcription middleware Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * feat: wire attachment processing into LLM pipeline with multimodal image support Attachments on incoming messages are now augmented into user text via XML tags before entering the turn system, and images with data are passed as multimodal content parts (base64 data URIs) to LLM providers. This enables audio transcripts, document text, and image content to reach the LLM without changes to ChatMessage serialization or provider interfaces. - Add src/agent/attachments.rs with augment_with_attachments() and 9 unit tests - Add ContentPart/ImageUrl types to llm::provider with OpenAI-compatible serde - Carry image_content_parts transiently on Turn (skipped in serialization) - Update nearai_chat and rig_adapter to serialize multimodal content - Add 3 e2e tests verifying attachments flow through the full agent loop Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: CI failures — formatting, version bumps, and Telegram voice test - Fix cargo fmt formatting in attachments.rs, nearai_chat.rs, rig_adapter.rs, e2e_attachments.rs - Bump channel registry versions 0.1.0 → 0.2.0 (discord, slack, telegram, whatsapp) to satisfy version-bump CI check - Fix Telegram test_extract_attachments_voice: add missing required `duration` field to voice fixture JSON Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: bump WIT channel version to 0.3.0, fix Telegram voice test, add pre-commit hook - Bump wit/channel.wit package version 0.2.0 → 0.3.0 (interface changed with store-attachment-data) - Update WIT_CHANNEL_VERSION constant and registry wit_version fields to match - Fix Telegram test_extract_attachments_voice: gate voice download behind #[cfg(target_arch = "wasm32")] so host functions aren't called in native tests, update assertions for generated filename and extras_json duration - Add @0.3.0 linker stubs in wit_compat.rs - Add .githooks/pre-commit hook that runs scripts/check-version-bumps.sh when WIT or extension sources are staged - Symlink commit-msg regression hook into .githooks/ [skip-regression-check] Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * refactor: extract voice download from extract_attachments into handle_message Move download_voice_file + store_attachment_data calls out of extract_attachments into a separate download_and_store_voice function called from handle_message. This keeps extract_attachments as a pure data-mapping function with no host calls, making it fully testable in native unit tests without #[cfg(target_arch)] gates. [skip-regression-check] Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: address PR review comments — security, correctness, and code quality Security fixes: - Add path validation to read_attachments (restrict to /tmp/) preventing arbitrary file reads from compromised tools - Escape XML special characters in attachment filenames, MIME types, and extracted text to prevent prompt injection via tag spoofing - Percent-encode file_id in Telegram getFile URL to prevent query injection - Clone SecretString directly instead of expose_secret().to_string() Correctness fixes: - Fix store_attachment_data overwrite accounting: subtract old entry size before adding new to prevent inflated totals and false rejections - Use max(reported, stored_size) for attachment size accounting to prevent WASM channels from under-reporting size_bytes to bypass limits - Add application/octet-stream to MIME allowlist (channels default unknown types to this) Code quality: - Extract send_response helper in Telegram, deduplicating on_respond and on_broadcast - Rename misleading Discord test to test_parse_slash_command_interaction - Fix .githooks/commit-msg to use relative symlink (portable across machines) [skip-regression-check] Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * feat: add tool_upgrade command + fix TOCTOU in save_to path validation Add `tool_upgrade` — a new extension management tool that automatically detects and reinstalls WASM extensions with outdated WIT versions. Preserves authentication secrets during upgrade. Supports upgrading a single extension by name or all installed WASM tools/channels at once. Fix TOCTOU in `validate_save_to_path`: validate the path *before* creating parent directories, so traversal paths like `/tmp/../../etc/` cannot cause filesystem mutations outside /tmp before being rejected. [skip-regression-check] Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: unify WIT package version to 0.3.0 across tool.wit and all capabilities tool.wit and channel.wit share the `near:agent` package namespace, so they must declare the same version. Bumps tool.wit from 0.2.0 to 0.3.0 and updates all capabilities files and registry entries to match. Fixes `cargo component build` failure: "package identifier near:agent@0.2.0 does not match previous package name of near:agent@0.3.0" [skip-regression-check] Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: move WIT file comments after package declaration WIT treats `//` comments before `package` as doc comments. When both tool.wit and channel.wit had header comments, the parser rejected them as "doc comments on multiple 'package' items". Move comments after the package declaration in both files. Also bumps tool registry versions to 0.2.0 to match the WIT 0.3.0 bump. [skip-regression-check] Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * feat: display extension versions in gateway Extensions tab Add version field to InstalledExtension and RegistryEntry types, pipe through the web API (ExtensionInfo, RegistryEntryInfo), and render as a badge in the gateway UI for both installed and available extensions. For installed WASM extensions, version is read from the capabilities file with a fallback to the registry entry when the local file has no version (old installations). Bump all extension Cargo.toml and registry JSON versions from 0.1.0 to 0.2.0 to keep them in sync. [skip-regression-check] Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * feat: add document text extraction middleware for PDF, Office, and text files Extract text from document attachments (PDF, DOCX, PPTX, XLSX, RTF, plain text, code files) so the LLM can reason about uploaded documents. Uses pdf-extract for PDFs, zip+XML parsing for Office XML formats, and UTF-8 decode for text files. Wired into the agent loop after transcription middleware. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: download document files in Telegram channel for text extraction The DocumentExtractionMiddleware needs file bytes in the attachment `data` field, but only voice files were being downloaded. Document attachments (PDFs, DOCX, etc.) had empty `data` and a source_url with a credential placeholder that only works inside the WASM host's http_request. Add `download_and_store_documents()` that downloads non-voice, non-image, non-audio attachments via the existing two-step getFile→download flow and stores bytes via `store_attachment_data` for host-side extraction. Also rename `download_voice_file` → `download_telegram_file` since it's generic for any file_id. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: allow Office MIME types and increase file download limit for Telegram Two issues preventing document extraction from Telegram: 1. PPTX/DOCX/XLSX MIME types (application/vnd.*) were dropped by the WASM host attachment allowlist — add application/vnd., application/msword, and application/rtf prefixes. 2. Telegram file downloads over 10 MB failed with "Response body too large" — set max_response_bytes to 20 MB in Telegram capabilities. [skip-regression-check] Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: report document extraction errors back to user instead of silently skipping - Bump max_response_bytes to 50 MB for Telegram file downloads - When document extraction fails (too large, download error, parse error), set extracted_text to a user-friendly error message instead of leaving it None. This ensures the LLM tells the user what went wrong. - On Telegram download failure, set extracted_text with the error so the user sees feedback even when the file never reaches the extraction middleware. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * feat: store extracted document text in workspace memory for search/recall After document extraction succeeds, write the extracted text to workspace memory at `documents/{date}/{filename}`. This enables: - Full-text and semantic search over past uploaded documents - Cross-conversation recall ("what did that PDF say?") - Automatic chunking and embedding via the workspace pipeline Documents are stored with metadata header (uploader, channel, date, MIME type). Error messages (extraction failures) are not stored — only successful extractions. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: CI failures — formatting, unused assignment warning - Run cargo fmt on document_extraction and agent_loop modules - Suppress unused_assignments warning on trace_llm_ref (used only behind #[cfg(feature = "libsql")]) [skip-regression-check] Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: address PR review comments — security, correctness, and code quality Security fixes: - Remove SSRF-prone download() from DocumentExtractionMiddleware (#13) - Sanitize filenames in workspace path to prevent directory traversal (#11) - Pre-check file size before reading in WASM wrapper to prevent OOM (#2) - Percent-encode file_id in Telegram source URLs (#7) Correctness fixes: - Clear image_content_parts on turn end to prevent memory leak (#1) - Find first *successful* transcription instead of first overall (#3) - Enforce data.len() size limit in document extraction (#10) - Use UTF-8 safe truncation with char_indices() (#12) Robustness & code quality: - Add 120s timeout to OpenAI Whisper HTTP client (#5) - Trim trailing slash from Whisper base_url (#6) - Allow ~/.ironclaw/ paths in WASM wrapper (#8) - Return error from on_broadcast in Slack/Discord/WhatsApp (#9) - Fix doc comment in HTTP tool (#4) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: formatting — cargo fmt Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: address latest PR review — doc comments, error messages, version bumps - Fix DocumentExtractionMiddleware doc comment (no longer downloads from source_url) - Fix error message: "no inline data" instead of "no download URL" - Log error + fallback instead of silent unwrap_or_default on Whisper HTTP client - Bump all capabilities.json versions from 0.1.0 to 0.2.0 to match Cargo.toml Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: remove unsupported profile: minimal from CI workflows [skip-regression-check] dtolnay/rust-toolchain@stable does not accept the 'profile' input (it was a parameter for the deprecated actions-rs/toolchain action). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: merge with latest main — resolve compilation errors and PR review nits - Add version: None to RegistryEntry/InstalledExtension test constructors - Fix MessageContent type mismatches in nearai_chat tests (String → MessageContent::Text) - Fix .contains() calls on MessageContent — use .as_text().unwrap() - Remove redundant trace_llm_ref = None assignment in test_rig - Check data size before clone in document extraction to avoid unnecessary allocation [skip-regression-check] Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
ilblackdragon
added a commit
that referenced
this pull request
Mar 9, 2026
* feat: add AWS Bedrock LLM provider via native Converse API * fix: use JSON parsing for tool result error detection instead of brittle substring matching * refactor: extract duplicated inference config builder into helper function * fix: address review feedback — safe casts, input validation, and tests - Safe u32→i32 cast for max_tokens using try_from with clamp - Remove brittle string-based error detection fallback for tool results - Validate BEDROCK_CROSS_REGION against allowed values (us/eu/apac/global) - Validate message list is non-empty before Converse API call - Log when using default us-east-1 region - Update llm_backend doc comment to list all backends - Add tests for build_inference_config and empty message handling * fix: persist AWS_PROFILE for Bedrock named profile auth The wizard collected the profile name but only printed a hint to set it manually. Now it saves to settings and writes AWS_PROFILE to the bootstrap .env, consistent with how BEDROCK_REGION and other Bedrock settings are persisted. * feat: gate AWS Bedrock behind optional `bedrock` feature flag The AWS SDK dependencies (aws-config, aws-sdk-bedrockruntime, aws-smithy-types) require cmake and a C compiler to build aws-lc-sys. Gate them behind an opt-in `bedrock` feature flag so default builds are unaffected. Build with: cargo build --features bedrock All config, settings, and wizard code stays unconditional (no AWS deps) so users can configure Bedrock even without the feature compiled — they get a clear error at startup directing them to rebuild. * fix: address review feedback and adapt Bedrock provider to registry architecture (takeover #345) - Resolve merge conflicts with main's registry-based provider system - Add missing cache_creation_input_tokens/cache_read_input_tokens fields - Add missing content_parts field in test ChatMessage - Fix string literal type mismatches in wizard env_vars (.to_string()) - Remove non-functional bearer token auth (AWS_BEARER_TOKEN_BEDROCK) from wizard and documentation per reviewer feedback from @zmanian and @serrrfirat - Remove stale BEDROCK_ACCESS_KEY proxy entry from provider table - Update Bedrock provider to use is_bedrock string check (LlmBackend enum removed) - Add bedrock_profile fallback from settings in config resolution [skip-regression-check] Co-Authored-By: cgorski <cgorski@users.noreply.github.com> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: use main's Cargo.lock as base to preserve dependency versions Regenerating Cargo.lock from scratch caused transitive dependency version drift that broke the html_to_markdown fixture test in CI. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: bedrock config bugs — spurious warning, alias normalization, profile fallback - Move is_bedrock check before unknown-backend warning to prevent spurious "unknown backend" log for bedrock users - Normalize backend aliases ("aws", "aws_bedrock") to "bedrock" so the provider factory matches correctly - Add settings.bedrock_profile fallback for AWS_PROFILE, consistent with region and cross_region resolution [skip-regression-check] Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: address Copilot review feedback — bearer token cleanup, stop_sequences, model dedup - Remove stale bearer token refs from setup README and CHANGELOG - Remove dead bedrock_api_key secret injection mapping - Pass stop_sequences through to Bedrock InferenceConfiguration - Remove "API key" from wizard menu description (bearer token removed) - Skip duplicate LLM_MODEL write for bedrock backend in wizard - Fix cargo fmt formatting [skip-regression-check] Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: address review feedback — async new(), remove LiteLLM entry, wizard fixes - Remove dead LiteLLM-based bedrock entry from providers.json (native Converse API intercepts before registry lookup) - Make BedrockProvider::new() async to avoid block_in_place panic in current_thread runtimes; propagate async to create_llm_provider, build_provider_chain, and init_llm - Document CMake build prerequisite in docs/LLM_PROVIDERS.md - Clear bedrock_profile when user selects "default credentials" in wizard - Fix selected_model clearing to match established pattern (conditional on provider switch, not unconditional) - Add regression tests for bedrock model preservation and profile clearing Addresses review feedback from @zmanian on PR #713. Streaming support tracked in #741. [skip-regression-check] Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: address remaining review comments — CLAUDE.md backends, wizard UX - Add `bedrock` to CLAUDE.md inline backend list (#10) - Skip full setup re-run when keeping existing Bedrock config (#11) - Clear stale bedrock_profile on empty named-profile input (#12) - Add regression test for empty profile clearing Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Chris Gorski <cgorski@cgorski.org> Co-authored-by: cgorski <cgorski@users.noreply.github.com> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
7 tasks
bkutasi
pushed a commit
to bkutasi/ironclaw
that referenced
this pull request
Mar 28, 2026
* add missing type * prune * readme * minor * update to .ironclaw
bkutasi
pushed a commit
to bkutasi/ironclaw
that referenced
this pull request
Mar 28, 2026
…tion (nearai#238) * feat: add extension registry with metadata catalog, CLI, and onboarding integration Adds a central registry that catalogs all 14 available extensions (10 tools, 4 channels) with their capabilities, auth requirements, and artifact references. The onboarding wizard now shows installable channels from the registry and offers tool installation as a new Step 7. - registry/ folder with per-extension JSON manifests and bundle definitions - src/registry/ module: manifest structs, catalog loader, installer - `ironclaw registry list|info|install|install-defaults` CLI commands - Setup wizard enhanced: channels from registry, new extensions step (8 steps) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(setup): resolve workspace errors for tool crates and channels-only onboarding Tool crates in tools-src/ and channels-src/ failed `cargo metadata` during onboard install because Cargo resolved them as part of the root workspace. Add `[workspace]` table to each standalone crate and extend the root `workspace.exclude` list so they build independently. Channels-only mode (`onboard --channels-only`) failed with "Secrets not configured" and "No database connection" because it skipped database and security setup. Add `reconnect_existing_db()` to establish the DB connection and load saved settings before running channel configuration. Also improve the tunnel "already configured" display to show full provider details (domain, mode, command) instead of just the provider name. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(registry): address PR review feedback on installer and catalog - Use manifest.name (not crate_name) for installed filenames so discovery, auth, and CLI commands all agree on the stem (nearai#1) - Add AlreadyInstalled error variant instead of misleading ExtensionNotFound (nearai#2) - Add DownloadFailed error variant with URL context instead of stuffing URLs into PathBuf (nearai#3) - Validate HTTP status with error_for_status() before reading response bytes in artifact downloads (nearai#4) - Switch build_wasm_component to tokio::process::Command with status() so build output streams to the terminal (nearai#6) - Find WASM artifact by crate_name specifically instead of picking the first .wasm file in the release directory (nearai#7) - Add is_file() guard in catalog loader to skip directories (nearai#8) - Detect ambiguous bare-name lookups when both tools/<name> and channels/<name> exist, with get_strict() returning an error (nearai#9) - Fix wizard step_extensions to check tool.name for installed detection, consistent with the new naming (nearai#11, nearai#12) - Fix redundant closures and map_or clippy warnings in changed files Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(setup): restore DB connection fields after settings reload reconnect_postgres() and reconnect_libsql() called Settings::from_db_map() which overwrote database_url / libsql_path / libsql_url set from env vars. Also use get_strict() in cmd_info to surface ambiguous bare-name errors. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * style: fix clippy collapsible_if and print_literal warnings Collapse nested if-let chains and inline string literals in format macros to satisfy CI clippy lint checks (deny warnings). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(registry): prefer artifacts for install-defaults and improve dir lookup - InstallDefaults now defaults to downloading pre-built artifacts (matching `registry install` behavior), with --build flag for source builds. - find_registry_dir() walks up 3 ancestor levels from the exe and adds a CARGO_MANIFEST_DIR fallback, matching load_registry_catalog() logic. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
bkutasi
pushed a commit
to bkutasi/ironclaw
that referenced
this pull request
Mar 28, 2026
) * feat: add inbound attachment support to WASM channel system Add attachment record to WIT interface and implement inbound media parsing across all four channel implementations (Telegram, Slack, WhatsApp, Discord). Attachments flow from WASM channels through EmittedMessage to IncomingMessage with validation (size limits, MIME allowlist, count caps) at the host boundary. - Add `attachment` record to `emitted-message` in wit/channel.wit - Add `IncomingAttachment` struct to channel.rs and re-export - Add host-side validation (20MB total, 10 max, MIME allowlist) - Telegram: parse photo, document, audio, video, voice, sticker - Slack: parse file attachments with url_private - WhatsApp: parse image, audio, video, document with captions - Discord: backward-compatible empty attachments - Update FEATURE_PARITY.md section 7 - Add fixture-based tests per channel and host integration tests [skip-regression-check] Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * feat: integrate outbound attachment support and reconcile WIT types (nearai#409) Reconcile PR nearai#409's outbound attachment work with our inbound attachment support into a unified design: WIT type split: - `inbound-attachment` in channel-host: metadata-only (id, mime_type, filename, size_bytes, source_url, storage_key, extracted_text) - `attachment` in channel: raw bytes (filename, mime_type, data) on agent-response for outbound sending Outbound features (from PR nearai#409): - `on-broadcast` WIT export for proactive messages without prior inbound - Telegram: multipart sendPhoto/sendDocument with auto photo→document fallback for files >10MB - wrapper.rs: `call_on_broadcast`, `read_attachments` from disk, attachment params threaded through `call_on_respond` - HTTP tool: `save_to` param for binary downloads to /tmp/ (50MB limit, path traversal protection, SSRF-safe redirect following) - Message tool: allow /tmp/ paths for attachments alongside base_dir - Credential env var fallback in inject_channel_credentials Channel updates: - All 4 channels implement on_broadcast (Telegram full, others stub) - Telegram: polling_enabled config, adjusted poll timeout - Inbound attachment types renamed to InboundAttachment in all channels Tests: 1965 passing (9 new), 0 clippy warnings [skip-regression-check] Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * feat: add audio transcription pipeline and extensible WIT attachment design Add host-side transcription middleware (OpenAI Whisper) that detects audio attachments with inline data on incoming messages and transcribes them automatically. Refactor WIT inbound-attachment to use extras-json and a store-attachment-data host function instead of typed fields, so future attachment properties (dimensions, codec, etc.) don't require WIT changes that invalidate all channel plugins. - Add src/transcription/ module: TranscriptionProvider trait, TranscriptionMiddleware, AudioFormat enum, OpenAI Whisper provider - Add src/config/transcription.rs: TRANSCRIPTION_ENABLED/MODEL/BASE_URL - Wire middleware into agent message loop via AgentDeps - WIT: replace data + duration-secs with extras-json + store-attachment-data - Host: parse extras-json for well-known keys, merge stored binary data - Telegram: download voice files via store-attachment-data, add duration to extras-json, add /file/bot to HTTP allowlist, voice-only placeholder - Add reqwest multipart feature for Whisper API uploads - 5 regression tests for transcription middleware Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * feat: wire attachment processing into LLM pipeline with multimodal image support Attachments on incoming messages are now augmented into user text via XML tags before entering the turn system, and images with data are passed as multimodal content parts (base64 data URIs) to LLM providers. This enables audio transcripts, document text, and image content to reach the LLM without changes to ChatMessage serialization or provider interfaces. - Add src/agent/attachments.rs with augment_with_attachments() and 9 unit tests - Add ContentPart/ImageUrl types to llm::provider with OpenAI-compatible serde - Carry image_content_parts transiently on Turn (skipped in serialization) - Update nearai_chat and rig_adapter to serialize multimodal content - Add 3 e2e tests verifying attachments flow through the full agent loop Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: CI failures — formatting, version bumps, and Telegram voice test - Fix cargo fmt formatting in attachments.rs, nearai_chat.rs, rig_adapter.rs, e2e_attachments.rs - Bump channel registry versions 0.1.0 → 0.2.0 (discord, slack, telegram, whatsapp) to satisfy version-bump CI check - Fix Telegram test_extract_attachments_voice: add missing required `duration` field to voice fixture JSON Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: bump WIT channel version to 0.3.0, fix Telegram voice test, add pre-commit hook - Bump wit/channel.wit package version 0.2.0 → 0.3.0 (interface changed with store-attachment-data) - Update WIT_CHANNEL_VERSION constant and registry wit_version fields to match - Fix Telegram test_extract_attachments_voice: gate voice download behind #[cfg(target_arch = "wasm32")] so host functions aren't called in native tests, update assertions for generated filename and extras_json duration - Add @0.3.0 linker stubs in wit_compat.rs - Add .githooks/pre-commit hook that runs scripts/check-version-bumps.sh when WIT or extension sources are staged - Symlink commit-msg regression hook into .githooks/ [skip-regression-check] Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * refactor: extract voice download from extract_attachments into handle_message Move download_voice_file + store_attachment_data calls out of extract_attachments into a separate download_and_store_voice function called from handle_message. This keeps extract_attachments as a pure data-mapping function with no host calls, making it fully testable in native unit tests without #[cfg(target_arch)] gates. [skip-regression-check] Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: address PR review comments — security, correctness, and code quality Security fixes: - Add path validation to read_attachments (restrict to /tmp/) preventing arbitrary file reads from compromised tools - Escape XML special characters in attachment filenames, MIME types, and extracted text to prevent prompt injection via tag spoofing - Percent-encode file_id in Telegram getFile URL to prevent query injection - Clone SecretString directly instead of expose_secret().to_string() Correctness fixes: - Fix store_attachment_data overwrite accounting: subtract old entry size before adding new to prevent inflated totals and false rejections - Use max(reported, stored_size) for attachment size accounting to prevent WASM channels from under-reporting size_bytes to bypass limits - Add application/octet-stream to MIME allowlist (channels default unknown types to this) Code quality: - Extract send_response helper in Telegram, deduplicating on_respond and on_broadcast - Rename misleading Discord test to test_parse_slash_command_interaction - Fix .githooks/commit-msg to use relative symlink (portable across machines) [skip-regression-check] Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * feat: add tool_upgrade command + fix TOCTOU in save_to path validation Add `tool_upgrade` — a new extension management tool that automatically detects and reinstalls WASM extensions with outdated WIT versions. Preserves authentication secrets during upgrade. Supports upgrading a single extension by name or all installed WASM tools/channels at once. Fix TOCTOU in `validate_save_to_path`: validate the path *before* creating parent directories, so traversal paths like `/tmp/../../etc/` cannot cause filesystem mutations outside /tmp before being rejected. [skip-regression-check] Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: unify WIT package version to 0.3.0 across tool.wit and all capabilities tool.wit and channel.wit share the `near:agent` package namespace, so they must declare the same version. Bumps tool.wit from 0.2.0 to 0.3.0 and updates all capabilities files and registry entries to match. Fixes `cargo component build` failure: "package identifier near:agent@0.2.0 does not match previous package name of near:agent@0.3.0" [skip-regression-check] Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: move WIT file comments after package declaration WIT treats `//` comments before `package` as doc comments. When both tool.wit and channel.wit had header comments, the parser rejected them as "doc comments on multiple 'package' items". Move comments after the package declaration in both files. Also bumps tool registry versions to 0.2.0 to match the WIT 0.3.0 bump. [skip-regression-check] Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * feat: display extension versions in gateway Extensions tab Add version field to InstalledExtension and RegistryEntry types, pipe through the web API (ExtensionInfo, RegistryEntryInfo), and render as a badge in the gateway UI for both installed and available extensions. For installed WASM extensions, version is read from the capabilities file with a fallback to the registry entry when the local file has no version (old installations). Bump all extension Cargo.toml and registry JSON versions from 0.1.0 to 0.2.0 to keep them in sync. [skip-regression-check] Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * feat: add document text extraction middleware for PDF, Office, and text files Extract text from document attachments (PDF, DOCX, PPTX, XLSX, RTF, plain text, code files) so the LLM can reason about uploaded documents. Uses pdf-extract for PDFs, zip+XML parsing for Office XML formats, and UTF-8 decode for text files. Wired into the agent loop after transcription middleware. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: download document files in Telegram channel for text extraction The DocumentExtractionMiddleware needs file bytes in the attachment `data` field, but only voice files were being downloaded. Document attachments (PDFs, DOCX, etc.) had empty `data` and a source_url with a credential placeholder that only works inside the WASM host's http_request. Add `download_and_store_documents()` that downloads non-voice, non-image, non-audio attachments via the existing two-step getFile→download flow and stores bytes via `store_attachment_data` for host-side extraction. Also rename `download_voice_file` → `download_telegram_file` since it's generic for any file_id. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: allow Office MIME types and increase file download limit for Telegram Two issues preventing document extraction from Telegram: 1. PPTX/DOCX/XLSX MIME types (application/vnd.*) were dropped by the WASM host attachment allowlist — add application/vnd., application/msword, and application/rtf prefixes. 2. Telegram file downloads over 10 MB failed with "Response body too large" — set max_response_bytes to 20 MB in Telegram capabilities. [skip-regression-check] Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: report document extraction errors back to user instead of silently skipping - Bump max_response_bytes to 50 MB for Telegram file downloads - When document extraction fails (too large, download error, parse error), set extracted_text to a user-friendly error message instead of leaving it None. This ensures the LLM tells the user what went wrong. - On Telegram download failure, set extracted_text with the error so the user sees feedback even when the file never reaches the extraction middleware. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * feat: store extracted document text in workspace memory for search/recall After document extraction succeeds, write the extracted text to workspace memory at `documents/{date}/{filename}`. This enables: - Full-text and semantic search over past uploaded documents - Cross-conversation recall ("what did that PDF say?") - Automatic chunking and embedding via the workspace pipeline Documents are stored with metadata header (uploader, channel, date, MIME type). Error messages (extraction failures) are not stored — only successful extractions. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: CI failures — formatting, unused assignment warning - Run cargo fmt on document_extraction and agent_loop modules - Suppress unused_assignments warning on trace_llm_ref (used only behind #[cfg(feature = "libsql")]) [skip-regression-check] Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: address PR review comments — security, correctness, and code quality Security fixes: - Remove SSRF-prone download() from DocumentExtractionMiddleware (nearai#13) - Sanitize filenames in workspace path to prevent directory traversal (nearai#11) - Pre-check file size before reading in WASM wrapper to prevent OOM (nearai#2) - Percent-encode file_id in Telegram source URLs (nearai#7) Correctness fixes: - Clear image_content_parts on turn end to prevent memory leak (nearai#1) - Find first *successful* transcription instead of first overall (nearai#3) - Enforce data.len() size limit in document extraction (nearai#10) - Use UTF-8 safe truncation with char_indices() (nearai#12) Robustness & code quality: - Add 120s timeout to OpenAI Whisper HTTP client (nearai#5) - Trim trailing slash from Whisper base_url (nearai#6) - Allow ~/.ironclaw/ paths in WASM wrapper (nearai#8) - Return error from on_broadcast in Slack/Discord/WhatsApp (nearai#9) - Fix doc comment in HTTP tool (nearai#4) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: formatting — cargo fmt Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: address latest PR review — doc comments, error messages, version bumps - Fix DocumentExtractionMiddleware doc comment (no longer downloads from source_url) - Fix error message: "no inline data" instead of "no download URL" - Log error + fallback instead of silent unwrap_or_default on Whisper HTTP client - Bump all capabilities.json versions from 0.1.0 to 0.2.0 to match Cargo.toml Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: remove unsupported profile: minimal from CI workflows [skip-regression-check] dtolnay/rust-toolchain@stable does not accept the 'profile' input (it was a parameter for the deprecated actions-rs/toolchain action). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: merge with latest main — resolve compilation errors and PR review nits - Add version: None to RegistryEntry/InstalledExtension test constructors - Fix MessageContent type mismatches in nearai_chat tests (String → MessageContent::Text) - Fix .contains() calls on MessageContent — use .as_text().unwrap() - Remove redundant trace_llm_ref = None assignment in test_rig - Check data size before clone in document extraction to avoid unnecessary allocation [skip-regression-check] Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
bkutasi
pushed a commit
to bkutasi/ironclaw
that referenced
this pull request
Mar 28, 2026
* feat: add AWS Bedrock LLM provider via native Converse API * fix: use JSON parsing for tool result error detection instead of brittle substring matching * refactor: extract duplicated inference config builder into helper function * fix: address review feedback — safe casts, input validation, and tests - Safe u32→i32 cast for max_tokens using try_from with clamp - Remove brittle string-based error detection fallback for tool results - Validate BEDROCK_CROSS_REGION against allowed values (us/eu/apac/global) - Validate message list is non-empty before Converse API call - Log when using default us-east-1 region - Update llm_backend doc comment to list all backends - Add tests for build_inference_config and empty message handling * fix: persist AWS_PROFILE for Bedrock named profile auth The wizard collected the profile name but only printed a hint to set it manually. Now it saves to settings and writes AWS_PROFILE to the bootstrap .env, consistent with how BEDROCK_REGION and other Bedrock settings are persisted. * feat: gate AWS Bedrock behind optional `bedrock` feature flag The AWS SDK dependencies (aws-config, aws-sdk-bedrockruntime, aws-smithy-types) require cmake and a C compiler to build aws-lc-sys. Gate them behind an opt-in `bedrock` feature flag so default builds are unaffected. Build with: cargo build --features bedrock All config, settings, and wizard code stays unconditional (no AWS deps) so users can configure Bedrock even without the feature compiled — they get a clear error at startup directing them to rebuild. * fix: address review feedback and adapt Bedrock provider to registry architecture (takeover nearai#345) - Resolve merge conflicts with main's registry-based provider system - Add missing cache_creation_input_tokens/cache_read_input_tokens fields - Add missing content_parts field in test ChatMessage - Fix string literal type mismatches in wizard env_vars (.to_string()) - Remove non-functional bearer token auth (AWS_BEARER_TOKEN_BEDROCK) from wizard and documentation per reviewer feedback from @zmanian and @serrrfirat - Remove stale BEDROCK_ACCESS_KEY proxy entry from provider table - Update Bedrock provider to use is_bedrock string check (LlmBackend enum removed) - Add bedrock_profile fallback from settings in config resolution [skip-regression-check] Co-Authored-By: cgorski <cgorski@users.noreply.github.com> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: use main's Cargo.lock as base to preserve dependency versions Regenerating Cargo.lock from scratch caused transitive dependency version drift that broke the html_to_markdown fixture test in CI. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: bedrock config bugs — spurious warning, alias normalization, profile fallback - Move is_bedrock check before unknown-backend warning to prevent spurious "unknown backend" log for bedrock users - Normalize backend aliases ("aws", "aws_bedrock") to "bedrock" so the provider factory matches correctly - Add settings.bedrock_profile fallback for AWS_PROFILE, consistent with region and cross_region resolution [skip-regression-check] Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: address Copilot review feedback — bearer token cleanup, stop_sequences, model dedup - Remove stale bearer token refs from setup README and CHANGELOG - Remove dead bedrock_api_key secret injection mapping - Pass stop_sequences through to Bedrock InferenceConfiguration - Remove "API key" from wizard menu description (bearer token removed) - Skip duplicate LLM_MODEL write for bedrock backend in wizard - Fix cargo fmt formatting [skip-regression-check] Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: address review feedback — async new(), remove LiteLLM entry, wizard fixes - Remove dead LiteLLM-based bedrock entry from providers.json (native Converse API intercepts before registry lookup) - Make BedrockProvider::new() async to avoid block_in_place panic in current_thread runtimes; propagate async to create_llm_provider, build_provider_chain, and init_llm - Document CMake build prerequisite in docs/LLM_PROVIDERS.md - Clear bedrock_profile when user selects "default credentials" in wizard - Fix selected_model clearing to match established pattern (conditional on provider switch, not unconditional) - Add regression tests for bedrock model preservation and profile clearing Addresses review feedback from @zmanian on PR nearai#713. Streaming support tracked in nearai#741. [skip-regression-check] Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: address remaining review comments — CLAUDE.md backends, wizard UX - Add `bedrock` to CLAUDE.md inline backend list (nearai#10) - Skip full setup re-run when keeping existing Bedrock config (nearai#11) - Clear stale bedrock_profile on empty named-profile input (nearai#12) - Add regression test for empty profile clearing Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Chris Gorski <cgorski@cgorski.org> Co-authored-by: cgorski <cgorski@users.noreply.github.com> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
7 tasks
17 tasks
9 tasks
horelvis
pushed a commit
to horelvis/jarvis-os
that referenced
this pull request
May 1, 2026
Per-client mpsc isolates each writer's pace from the SseManager broadcast bus. A subscriber that never reads from the socket only fills its own 16-slot buffer; the broadcast tx drops the lagged events for that subscriber and other clients keep flowing. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
zmanian
added a commit
that referenced
this pull request
May 15, 2026
Bundle three nit-tier review items into a single commit: **#9 Replace author-internal tags with NOTE(#3640)** The Phase-5 PR (#3640) had several `serrrfirat HIGH/MED #N on PR #3640` comment tags in this PR's diff. These are review-internal scaffolding, not load-bearing for future readers. Replaced with `NOTE(#3640)` in: - crates/ironclaw_hooks/src/dispatch.rs (self-observation guard) - crates/ironclaw_reborn/src/loop_driver_host.rs (scope validation, replay-gap milestone, subscription binding) - crates/ironclaw_reborn/tests/hooks_integration.rs (three regression tests covering scope validation, self-observation suppression, and replay-gap surfacing) - crates/ironclaw_turns/src/run_profile/host.rs (`EventSubscriptionTerminated` doc) **#10 Replace 10ms spin-poll with tokio::sync::Notify** `wait_for_seen_events` polled the shared `Mutex<Vec<SeenRuntimeEvent>>` every 10 ms until the expected count was reached. Replaced with a `SeenLog` newtype that pairs the events vec with a `Notify`; the hook's `observe()` calls `seen.push(...)` which signals `Notify::notify_one`, and `wait_for_seen_events` parks on `notified().await` under a `tokio::time::timeout`. `notify_one` is a permit-store, so an event landing between snapshot and wait still wakes the waiter immediately. Test latency drops from ~10 ms median to sub-ms and is no longer rate-limited by the polling cadence. All 30 hooks_integration tests still pass. **#11 Remove unused Clone derive on EventTriggeredHookContext** No call site clones the context — it's passed by reference. Dropped the derive to make the borrow contract clearer.
ilblackdragon
added a commit
that referenced
this pull request
May 16, 2026
…check Four follow-ups from review of 87fc8d4: - responses_api.rs (Copilot #11): the byte-counter comment claimed it measured "what the caller actually sent over the wire" but the count is canonicalised JSON, not raw request bytes. Rewrote the comment to describe the canonicalised-size cap correctly. The `Serializer` import is kept — it's needed to bring the trait method into scope for the concrete `serde_json::Serializer::serialize_seq` call below (an earlier attempt to drop it failed to compile). - responses_api.rs (Copilot #12): caller-supplied tool names were only checked for non-empty + uniqueness. Whitespace, control chars, and over-long names could propagate into engine action surfaces, SSE payloads, and downstream LLM clients (which all enforce the OpenAI Responses spec `^[A-Za-z0-9_-]{1,64}$` and would reject anyway). Validate at request time. Added regression tests covering whitespace/control/non-ASCII names and the length cap. - responses_api.rs / bridge/router.rs / bridge/mod.rs (Copilot #13): the shadow check only consulted `ToolRegistry::tool_definitions()` and missed engine v2 capability actions (`mission_*`, `skill_*`, `memory_*`, etc.). A caller registering `mission_create` as an external tool would still hit the catalog short-circuit in `EffectBridgeAdapter::execute_action`, since the LLM-visible dedup in `available_action_inventory` doesn't extend to the execute path. Added `engine_capability_action_names()` accessor that pulls the full capability-action surface from the bridge's `CapabilityRegistry`, and merged it into the collision set the responses_api handler checks against. - runtime/conversation.rs (Copilot #14): documented that `extra_initial_metadata` is spawn-only. The `Running` (inject) and `Resumable` (resume) branches ignore it; callers needing per-request metadata on existing threads must use `ThreadManager::set_thread_metadata` instead. The bridge's external-tool-catalog `transfer` already handles this for the one in-tree caller, but documenting the contract prevents future surprise.
ilblackdragon
added a commit
that referenced
this pull request
May 16, 2026
* feat(web): support externally-provided tools in Responses API Lets callers of `/v1/responses` (and `/api/v1/responses`) declare their own `function`-typed tools and feed back results via `function_call_output` items, matching the OpenAI Responses wire shape. Since IronClaw's engine has no per-request tool surface, integration happens at the prompt level: the catalog is rendered as `<external-tools>` in the user message and the agent signals a call by ending its response with a fenced ```` ```tool_call ```` block. When that fence is recognised, the reply is split into a leading `Message` plus a `function_call` `ResponseOutputItem`. Validation rejects unsupported tool types (`web_search`, `file_search`, `code_interpreter`) and tools missing `name` with 400, with two new integration tests covering both paths. * refactor(responses-api): switch external tools to engine v2 native path Replace the prompt-level fence protocol from PR #3122 with engine v2 native tool calls: caller-supplied `tools[]` are surfaced as real LLM-callable actions, the engine pauses with `ResumeKind::External` when one is invoked, and the bridge router projects the pause to a new `AppEvent::ExternalToolCall` carrying the OpenAI-shaped `function_call` wire fields. The integration is small because v2 already has the right primitives: - `ResumeKind::External { callback_id }` and `GateResolution::ExternalCallback { payload }` already existed for OAuth-style callbacks. - `agent_loop.rs:1480` already routes Responses API messages to `handle_with_engine` when `ENGINE_V2=true`, so no v2 migration of the endpoint itself is needed. - `EffectBridgeAdapter::execute_action` is the single chokepoint where caller tools can be detected before they reach the dispatch pipeline. Changes: - New `src/bridge/external_tools.rs` (`ExternalToolCatalog`) — per-thread registry of caller-supplied `ActionDef`s, plus the `ext_tool:` callback-id helpers used to disambiguate external-tool pauses from OAuth/pairing pauses (which also use `ResumeKind::External`). - `EffectBridgeAdapter` consults the catalog: any name in it is short-circuited to a `GatePaused { resume_kind: External { callback_id: ext_tool:<call_id> } }` before any registry dispatch, and `available_action_inventory` merges the catalog into the LLM-visible action surface (internal beats external on collision). - `Submission::ExternalCallback` gains an optional `payload` field; `bridge::handle_external_callback` plumbs it into `GateResolution::ExternalCallback { payload }`. Fallback predicate `gate_resume_is_external` lets non-auth External pauses (i.e. caller-tool resumes) resolve through the same handler. - New `AppEvent::ExternalToolCall` projected by `notify_pending_gate` when a paused gate carries an `ext_tool:` callback id; OAuth/ pairing flows keep flowing through the existing `GateRequired` channel. - `responses_api.rs` is gutted of the prompt rendering and fence parsing (`render_external_tools_preamble`, `extract_trailing_tool_call`, `parse_external_tool_call`, `ParsedToolCall`, `external_tool_names` accumulator field, and the `TOOL_CALL_FENCE` constants). The handler now: rejects `tools[]` when `ENGINE_V2=false`, registers caller tools in the catalog under the resolved thread id, detects resume requests (`previous_response_id` + `function_call_output` items in `input`) and submits them as `Submission::ExternalCallback` with the outputs as the resolution payload, and surfaces `AppEvent::ExternalToolCall` as a `function_call` `ResponseOutputItem` in both streaming (`output_item.added`+`done`) and non-streaming. - All existing OAuth/pairing `ExternalCallback` constructors updated to pass `payload: None` (no behaviour change). - Fence-protocol unit tests removed; replaced with coverage for the new `responses_tools_to_action_defs` converter and the accumulator's `ExternalToolCall` arm. Existing 9 integration tests in `tests/responses_api_path_prefix.rs` still pass. Note for reviewers: - The accumulator-side text response no longer tries to split the reply on a fenced `tool_call` block. The wire shape that callers receive for caller-tool invocations is purely event-driven now. - Internal vs external collision is handled silently by the dedup in `available_action_inventory` (internal wins). A request-time rejection for shadowing names is a follow-up — the current behavior is safe (the LLM only sees the internal version) but could surprise a caller who expects their tool to run. * test(responses-api): cover ENGINE_V2-off and resume-without-pending-gate Two new integration tests for behaviours added by the engine-native external-tool refactor: - `external_tools_rejected_when_engine_v2_disabled`: a request with caller-supplied `tools[]` while `ENGINE_V2` is off must 400 with a message naming the flag, not silently fall through. - `resume_without_pending_gate_returns_400`: a request with `function_call_output` items and a `previous_response_id` that doesn't correspond to a live external-tool gate must 400, not start a fresh turn against the (unrelated) thread. Both tests drive the full router (`start_test_server` + bearer auth) per `.claude/rules/testing.md` "Test Through the Caller". * test(responses-api): integration tests + drop unsafe env mutation Three groups of changes: 1. **Drop unsafe env-var mutation in tests.** `responses_api.rs` no longer reads `ENGINE_V2` directly: it keys off the presence of the live `ExternalToolCatalog` (initialized by `init_engine`) as the "engine v2 is up" signal. The path-prefix test that exercises the no-engine branch no longer needs `unsafe { std::env::remove_var }` — the absence of `init_engine` in `TestGatewayBuilder` is what makes the catalog absent, which is what makes the request reject. 2. **Engine-tier integration tests** (`tests/e2e_responses_api_external_tools.rs`). Drives engine v2 via the existing `TestRigBuilder` + `TraceLlm` replay infrastructure rather than spinning up an HTTP gateway: - `catalog_isolates_by_thread_id` — register-under-A doesn't bleed into B. - `catalog_register_overwrites_not_merges` — Responses API contract is "each request restates the full tools[]"; the catalog must replace, not merge. - `catalog_sweep_evicts_only_stale_entries` — TTL backstop. - `catalog_clear_on_terminal_state_explicit` — what we want; the test name flags that the production hook is missing. - `catalog_handles_concurrent_registrations` — 32 concurrent register-then-contains tasks; protects per-thread isolation under contention. - `callback_id_disambiguates_external_from_oauth` — `ext_tool:` vs `pairing:` prefix is the single bit that routes a paused gate to `AppEvent::ExternalToolCall` vs `AppEvent::GateRequired`. If the prefix invariant breaks, the wrong UI renders. Two more tests deliberately `#[ignore]` and document concrete bugs the implementation has today: - `engine_pauses_when_llm_calls_registered_external_tool` — running it surfaces "engine never paused on external tool". Confirms the **thread-id mismatch** bug: the catalog is keyed by engine `ThreadId`, but the responses_api handler registers under a separately-generated UUID before the engine spawns the thread. - `round_trip_resume_payload_reaches_llm` — the load-bearing end-to-end test. Documents the **resume-payload-not-materialised** bug: `bridge::router::resolve_gate` uses `pending.resume_output` for `ExternalCallback` resolutions and ignores the payload, so caller-supplied tool outputs never reach the LLM's context. - `external_collision_with_registry_action_is_rejected` — stub asserting the desired validation behaviour for caller tool names that shadow registry actions; today silently accepted, and the catalog-wins-in-dispatch ordering means the LLM thinks it called the internal tool but actually ran caller code. The two #[ignore] tests are the deliberate failure documentation: running them with `--ignored` panics with messages naming the underlying gap. The fixes go on a follow-up commit. 3. **`TestRigBuilder.send_external_callback_with_payload`** — new helper that mirrors the OAuth `send_external_callback` but carries a JSON payload. Used by the round-trip test; the existing payload-less variant kept for OAuth/pairing tests. Quality gates: `cargo fmt`, `cargo clippy --all --tests --all-features` clean. 14 of 17 tests pass; the 3 ignored ones are deliberate documentation of the gaps. * fix(responses-api): close 4 bugs surfaced by integration tests The engine-native external-tools path landed in 44135ca had four real bugs surfaced by the integration tests in a2c13ee. This commit fixes all four; every previously-ignored test now passes. **Bug 1 — Thread-id mismatch.** `responses_api.rs` registers tools in the catalog under a `thread_uuid` it generates from `previous_response_id` (or freshly), but `ConversationManager::handle_user_message` creates the engine's *actual* `ThreadId` internally. The catalog entries were under a UUID the engine never executed in. Fix: `ExternalToolCatalog::transfer(from, to)` and a hook in `bridge::handle_with_engine_inner` that calls it after the engine returns the spawned `ThreadId`. The handler-supplied conversation scope UUID is rebound onto the actual ThreadId before the LLM call lands, so `EffectBridgeAdapter::execute_action`'s catalog check finds the registered tools. **Bug 2 — Resume payload never materialised.** `bridge::router::resolve_gate`'s `GateResolution::ExternalCallback` branch only consulted `pending.resume_output` to construct the resumed `ActionResult`. `EffectBridgeAdapter::execute_action` sets that to `None` for caller-tool gates (the output isn't known at gate-fire time), so the resume fell through to `execute_pending_gate_action`, which re-ran the original action — re-pausing forever. The caller's tool output (passed via `GateResolution::ExternalCallback { payload }`) was dropped on the floor. Fix: special-case `ext_tool:` callback ids in `resolve_gate`'s ExternalCallback branch. Extract the matching output from the payload (Responses API wire shape: `{ outputs: [{ call_id, output }] }`) via the new `extract_external_tool_output` helper, synthesise an `ActionResult`-shaped ThreadMessage, and resume the thread directly. OAuth/pairing flows (which use `pairing:` callback ids and don't carry an output payload) keep the original `pending.resume_output` path unchanged. **Bug 3 — Internal/external collision in dispatch.** The catalog short-circuit in `EffectBridgeAdapter::execute_action` ran before the registry, but `available_action_inventory` dedupes the opposite way (internal beats external in the LLM-visible list). Result: an LLM call to (say) `shell` would land in caller-side execution even though the LLM saw the *internal* `shell` description in its action surface — a confused-deputy where the caller can return any output and the LLM trusts it as the internal tool's reply. Fix: reject the collision at request validation in `responses_api.rs`. After `validate_external_tools(...)`, look up registered tool names via `state.tool_registry.tool_definitions()` and 400 any caller name that shadows an internal action. **Bug 4 — Catalog cleanup never happens.** `sweep_older_than` existed but nothing scheduled it, and there was no terminal-state hook — so catalog entries accumulated for every thread that ever ran. Fix: - `await_thread_outcome` in the bridge router calls `catalog.clear(thread_id)` on every non-`GatePaused` outcome (Completed, Stopped, MaxIterations, Failed). `GatePaused` keeps the entry so resume requests can still find it. - A periodic sweep task in `init_engine` runs `catalog.sweep_older_than(1 hour)` every 5 minutes as a backstop for callers that abandon a paused thread without resuming. **Tests now passing:** - `engine_pauses_when_llm_calls_registered_external_tool` — proves Bug 1. - `round_trip_resume_payload_reaches_llm` — proves Bug 2 (and Bug 1 by extension). - `external_tool_name_shadowing_registered_action_is_rejected` — proves Bug 3. - `catalog_cleared_on_terminal_completed_outcome` — proves Bug 4. Plus four catalog-tier unit tests for the new `transfer` method, and the two pre-existing collision-prevention tests (`ext_tool:` vs `pairing:` callback id disambiguation). `TestGatewayBuilder.tool_registry(...)` is a new builder hook for tests that need to exercise the registry-aware handler paths (currently only the collision-rejection test, but the seam is there for future ones). Quality gates: `cargo fmt`, `cargo clippy --all --benches --tests --examples --all-features` clean. 17/17 engine v2 tests pass; 12/12 HTTP path-prefix tests pass. * fix(responses-api): address review feedback from PR #3122 Closes the race-window where caller-supplied external tools could be invisible to the LLM on a thread's first turn, plus a batch of smaller review findings. Race fix (Bug #3 from review): - Plumb `conversation_scope: Option<Uuid>` through `ThreadExecutionContext`, populated from thread metadata. - `ConversationManager::handle_user_message` accepts an `extra_initial_metadata` map; the bridge stamps the parsed scope into it so the engine sees it on the in-memory thread the executor task reads from (post-spawn `set_thread_metadata` is invisible to that task). - `EffectBridgeAdapter::execute_action` and `available_action_inventory` now look up the catalog under both `ctx.thread_id` and `ctx.conversation_scope`, so the executor task that starts immediately after spawn can find caller tools even before the bridge's post-spawn `transfer` rebinds them onto the engine `thread_id`. The transfer remains for terminal-state cleanup bookkeeping. Other review fixes: - Rewrite the stale `## Externally-provided tools` module doc in `responses_api.rs` to describe the engine-native flow (the original prompt-level fence text was left over from the first commit on the branch). - `validate_external_tools` size check now fails closed on serialization error (`unwrap_or(MAX + 1)`) instead of silently passing oversized payloads. - `notify_pending_gate` debug-logs the no-broadcaster path so a future SSE-less channel that grows an external-tool surface can be diagnosed instead of silently hanging. - Update `catalog_clear_on_terminal_state_explicit` test docstring and rename to `catalog_clear_removes_entry` — Bug 4's cleanup hook in `await_thread_outcome` already wires the production cleanup (covered separately by `catalog_cleared_on_terminal_completed_outcome`). - Delete dead `wait_for_first_engine_thread` helper and the `_harness_compiles` shim that kept it alive. New test coverage: - `bridge::effect_adapter::tests`: three race-window regression tests exercising both `available_action_inventory` and `execute_action` via the conversation_scope fallback path, plus a unit test on the `external_tool_catalog_keys` helper. - `bridge::router::tests`: four `extract_external_tool_output` tests covering match-by-call_id, missing-call_id-returns-null, no-outputs-array fallback, and find-after-misses. - `channels::web::responses_api::tests`: a full streaming round-trip test driving `streaming_worker` end-to-end with synthetic StreamChunk + ExternalToolCall events, parsing the actual SSE byte stream, and asserting the wire-frame ordering an OpenAI client would observe. * test: fix three pre-existing engine test failures - `executor::structured::call_id_preserved_when_no_lease`: the test was asserting on an error string ("no lease") that the preflight path stopped emitting when it added the "not callable in this execution context" check ahead of the lease lookup. The empty `MockEffects` used by the test exposed no actions, so the call short-circuited before reaching the lease check the test name describes. Register `web_search` in the inventory so the lease-miss path the test is named for actually fires. - `runtime::manager::stop_thread_works`: the test races the consecutive-action-error guard added in #2325. The thread loops on a deliberately unregistered `test_tool`, and 7 errors land before the 10ms sleep + `stop_thread` round-trip can deliver the stop signal in fast environments. The test's intent is "calling `stop_thread` doesn't deadlock the join", so accept `Failed` as a valid terminal outcome in addition to `Stopped`/`Completed`/ `MaxIterations`. Asserts a more specific message on the join result so a true regression (non-terminal outcome) still trips. - `tests::catalog_cleared_on_terminal_completed_outcome`: was racing two ways. (1) The cleanup-poll compared `catalog.len()` to a pre-snapshot — racy because `engine_external_tool_catalog` is process-global, so concurrent tests could keep the count from ever dropping below the pre-snapshot. (2) Multiple engine-touching tests in the file race the `OnceLock<Option<EngineState>>` global init, so messages sent by test A could be processed by test B's engine state. Fixes: - Add `ExternalToolCatalog::contains_action_anywhere(name)` so the cleanup poll can verify a unique marker action is gone regardless of what other tests have registered, and switch the test to use a per-test `format!("cleanup_marker_{uuid}")` action name. - Add a process-static `tokio::sync::Mutex` (`engine_state_lock`) that the three engine-touching tests in the file acquire for their duration, serializing their use of the global engine state. Per-test engine isolation belongs in the bridge itself; this is a test-side workaround until that lands. * fix(responses-api): close gemini bot review findings Three review fixes from gemini-code-assist on PR #3122 plus a related fix for fence-style tool calls reported by the user when running gpt-5.3-codex through `/v1/responses`. streaming_worker: finalize message item even when resolved text is empty ========================================================================= The Response branch only emitted `output_item.done` when the resolved text was non-empty. If `StreamChunks` had already opened a Message item (`output_item.added` fired, `message_output_index` is `Some`) and the terminal Response then resolved to an empty string, `output_item.done` was skipped — leaving the OpenAI client with a dangling in-progress message in its UI. Now we finalize whenever either text is available or a message item is in flight, and skip the redundant delta emit when there's nothing to deliver. Regression test `streaming_worker_finalizes_item_when_resolved_text_is_empty` drives the worker with an empty StreamChunk + empty Response and asserts `added_count == done_count` for output_item events. validate_external_tools: stream the size check, no allocation ========================================================================= Replaced the `Vec<Value>` + `String::len()` size measurement with a streaming `serde_json::Serializer` writing into a counting `io::Write` sink. Same byte count, no intermediate heap allocation, correctness contract unchanged (still fails closed if the serialization stream errors mid-flight). recover_tool_calls_from_content: recognize markdown-fenced tool calls ========================================================================= Some OpenAI-compatible models — notably gpt-5.3-codex via the Codex Responses API — emit caller-supplied tool calls as ```tool_call {"name": "get_balances", "arguments": {}} ``` instead of via the structured `function_call` output channel. Root cause is engine v2's CodeAct preamble pushing "always respond in a ```repl block" hard enough that the model generalizes the fenced protocol to a sibling `tool_call` fence for tools it can't dispatch through Python. Tools ARE passed to the provider correctly (`OpenAiCodexProvider::build_request_body` sets `body["tools"]` with strict-OpenAI schema and `tool_choice: "auto"`); the model just opts out of the structured surface in favor of a fence. The structural fix is to suppress the CodeAct preamble for Responses API turns that carry caller-supplied `tools[]`, but that's a larger piece of work. As a defense-in-depth fix: - `recover_tool_calls_from_content` now also matches markdown-fenced blocks with `tool_call`, `function_call`, or `tool_calls` info strings. Opening fence must be at line start to avoid matching inline backtick references inside prose. JSON body is parsed and validated against the available tool name set; unknown names are ignored. - `clean_response` strips the same fenced blocks via a new `strip_markdown_fence_block` helper so any malformed-JSON fence the recovery skipped doesn't leak fence syntax to the user. Six new unit tests in `llm::reasoning::tests` cover the recovery (JSON, with arguments, function_call alias, unknown-tool ignored, inline-reference ignored) and the clean_response stripping (clean case + malformed-JSON case). * fix(responses-api): emit ExternalToolCall on CodeAct GatePaused outcome The Responses API was timing out on caller-supplied tool calls when the LLM emitted them through CodeAct's Python (which is the default mode for engine v2). Symptoms reported: gateway shows 'Tool X requires external confirmation (gate: external_tool)' as a generic gate card and the /v1/responses POST returns response.failed. There are two paths that handle a `GatePaused { External }` from the engine: 1. `notify_pending_gate` — fires when the engine creates the gate mid-execution (Tier 0 structured tool calls). My earlier review- feedback fix wired this path to emit `AppEvent::ExternalToolCall` so the Responses API handler can surface a `function_call` ResponseOutputItem. 2. `await_thread_outcome`'s `ThreadOutcome::GatePaused` arm — fires when CodeAct's Python catches `EngineError::GatePaused`, raises a `RuntimeError("execution paused by gate 'external_tool'")`, the script unwinds, and the thread terminates with a `GatePaused` outcome. This arm calls `send_pending_gate_status`, which has an empty branch for `ResumeKind::External` (line 514: `External { .. } => {}`). No `AppEvent::ExternalToolCall` was emitted, so the Responses API handler waited for a never-arriving event and the response timed out as failed. Fix: in the `GatePaused` arm, when `resume_kind` is External with the `ext_tool:` callback prefix, broadcast `AppEvent::ExternalToolCall` through the SSE manager and short-circuit past the approval-card delivery path (which is for human-in-the-loop UX and doesn't apply to caller-executed tools). Mirrors the `notify_pending_gate` projection. Annotated with `// projection-exempt: bridge dispatcher, ...` per `.claude/rules/gateway-events.md` since the broadcast is a projection of a `ThreadOutcome` (one of the canonical source logs) rather than an unscheduled side-channel emit. Also update the `persist_v2_tool_calls_only_called_from_completed_arm` regression test, which was matching on bare `ThreadOutcome::Completed` and `ThreadOutcome::GatePaused` text. The Bug 4 catalog-cleanup hook (commit a4ba764) introduced an early `if !matches!(outcome, ThreadOutcome::GatePaused { .. })` guard above the match block, so the first text occurrence of `ThreadOutcome::GatePaused` is now in that guard rather than the match arm — false-failing the assertion. Anchor the test on the match-arm destructuring patterns (with first-field names) instead, so it pins the structural invariant the test name describes. * fix(responses-api): filter synthetic engine markers; clearer resume error Two issues from the user's live test of caller-supplied tools: 1. `__codeact__` was leaking to the response output as a `function_call` item with that name. The orchestrator emits ActionFailed events with `action_name: "__codeact__"` when a CodeAct script crashes (orchestrator.rs:940), and the responses_api accumulator + streaming worker were dutifully projecting those into `function_call` items. Fix: add `is_synthetic_engine_action(name)` (any `__double_underscore__` name) and skip those events in `ResponseAccumulator::process` and `streaming_worker` for the ToolStarted/ToolCompleted/ToolResult arms. Internal markers no longer surface to the caller. 2. The "function_call_output supplied but no pending external tool call" error was opaque — it gave the caller no way to diagnose why their resume failed. Most common cause: the LLM ran caller tools through CodeAct (Python) instead of structured tool calls, which currently doesn't pause the thread (script crashes with a Python RuntimeError, no GatePaused outcome, no PendingGate persisted). Engine-side fix is in PR #3157 and needs an extension to cover `External` resume kinds. Improved the error to point at: - The diagnostic check (verify prior response.output had a function_call item for this call_id) - The known limitation (CodeAct path doesn't dispatch caller tools) - PR #3157 as the in-progress engine fix Two new tests: - `accumulator_filters_synthetic_engine_actions`: drives ToolStarted + ToolCompleted with `name: "__codeact__"` and asserts the output array stays empty. - `is_synthetic_engine_action_recognizes_double_underscore`: pins the `__double_underscore__` predicate (positive: `__codeact__`, `__init__`; negative: regular names, single-underscore, leading- or trailing-only doubles). * fix(responses-api): tighten resume validation, sanitize external payloads, address review findings Addresses PR #3122 review comments plus the user-directed clean-up: - responses_api.rs: reject `function_call_output` items with missing/empty `call_id` (Copilot 474). Validate the pending gate is actually an external-tool gate (ResumeKind::External + `ext_tool:` prefix) and that at least one supplied `call_id` matches the pending callback before submitting the ExternalCallback (Copilot 1211). - bridge/router.rs: run the synthesized external-tool payload through `SafetyLayer::sanitize_tool_output` before it reaches the LLM — external tool payloads originate outside `EffectBridgeAdapter`'s pipeline and need the same leak/policy/sanitizer pass internal tool outputs get. Move projection-exempt annotations onto the `broadcast_for_user` call lines so the gateway-events check passes. - bridge/effect_adapter.rs: synthesize a `call_ext_<uuid>` call id when the executor reaches the external-tool short-circuit without `current_call_id` (Copilot effect_adapter.rs:1853), and document multi-call batching as an expected limitation at the short-circuit site so future readers know the engine pauses on first. - reasoning.rs: correct the stale fence-recovery comment (Copilot 1738). - e2e_responses_api_external_tools.rs: remove the "currently expected to fail" note; the test now pins the resume materialisation contract (Copilot 153). * fix(responses-api): finalize streaming placeholder before function_call; strip stale PR pointer Two follow-ups from review: 1. Streaming external-tool dangling `output_item.added`. When a StreamChunk arrived before the ExternalToolCall (the placeholder Message was already emitted via `output_item.added`), the prose-flush path created a *new* Message item at `acc.output.len()` and emitted a fresh added+done pair for it — never finalizing the original placeholder. OpenAI clients render the unmatched placeholder as "in progress" forever. Fix: when `message_output_index` is set, take it, fold the accumulated chunks into the existing item at that index, and emit `output_item.done` for the same index. Falls through to the original no-placeholder behaviour when there is no in-flight Message. Updated `streaming_worker_external_tool_call_emits_correct_frame_sequence` to match the corrected sequence (one Message added, one Message done, then FunctionCall added/done) and added a pairing-invariant assertion (`added_count == done_count`) so the regression can't be re-locked by an incorrect literal sequence. 2. Wire-visible error message in `responses_api.rs` referenced PR #3157 for the CodeAct fix. Once the PR merges the pointer is misleading, and external callers can't follow the link anyway. Dropped the trailing note; the behavioural part of the message stays. * fix(responses-api): tighten external tool name validation and shadow check Four follow-ups from review of 87fc8d4: - responses_api.rs (Copilot #11): the byte-counter comment claimed it measured "what the caller actually sent over the wire" but the count is canonicalised JSON, not raw request bytes. Rewrote the comment to describe the canonicalised-size cap correctly. The `Serializer` import is kept — it's needed to bring the trait method into scope for the concrete `serde_json::Serializer::serialize_seq` call below (an earlier attempt to drop it failed to compile). - responses_api.rs (Copilot #12): caller-supplied tool names were only checked for non-empty + uniqueness. Whitespace, control chars, and over-long names could propagate into engine action surfaces, SSE payloads, and downstream LLM clients (which all enforce the OpenAI Responses spec `^[A-Za-z0-9_-]{1,64}$` and would reject anyway). Validate at request time. Added regression tests covering whitespace/control/non-ASCII names and the length cap. - responses_api.rs / bridge/router.rs / bridge/mod.rs (Copilot #13): the shadow check only consulted `ToolRegistry::tool_definitions()` and missed engine v2 capability actions (`mission_*`, `skill_*`, `memory_*`, etc.). A caller registering `mission_create` as an external tool would still hit the catalog short-circuit in `EffectBridgeAdapter::execute_action`, since the LLM-visible dedup in `available_action_inventory` doesn't extend to the execute path. Added `engine_capability_action_names()` accessor that pulls the full capability-action surface from the bridge's `CapabilityRegistry`, and merged it into the collision set the responses_api handler checks against. - runtime/conversation.rs (Copilot #14): documented that `extra_initial_metadata` is spawn-only. The `Running` (inject) and `Resumable` (resume) branches ignore it; callers needing per-request metadata on existing threads must use `ThreadManager::set_thread_metadata` instead. The bridge's external-tool-catalog `transfer` already handles this for the one in-tree caller, but documenting the contract prevents future surprise.
zmanian
added a commit
that referenced
this pull request
May 20, 2026
…egistrar happy path Round out the test set for the WASM hook execution path: #11 / #12: gate + observer wall-clock timeout. The pre-fix dispatcher ran wasmtime synchronously on the executor, so the outer `tokio::time::timeout` `Err(_elapsed)` arm was effectively unreachable. Now that WASM execution runs on the blocking pool, the timeout actually fires; the new tests give the wasm budget headroom (1B fuel, 5s wall) and the dispatcher a 20 ms timeout, then assert the failure classification (FailClosed for gate, FailIsolated for observer). #13: observer memory exhaustion. Mirrors `wasm_memory_exhaustion_fails_closed_for_gate` against the observer dispatch path so the FailIsolated branch of the failure matrix has explicit memory coverage, not just fuel/wall. #15: `WasmResourceLimiter::memory_grow_failed` rollback. Stages an approved grow, simulates the OS-level grow failing, and asserts a subsequent grow of the full ceiling succeeds — the inflated `memory_used` from the failed attempt must be released. #16: registrar WASM happy path. Companion to the existing `install_wasm_body_requires_runtime` negative case: a valid module installs, the binding is visible via the public registry accessor, and is not pre-poisoned. #14 (`add_milestone_metadata` happy path) is intentionally omitted — the BeforePrompt dispatch path is currently unreachable due to a pre-existing manifest-vs-registry scope conflict (`OwnCapabilities` is the only valid `BeforePrompt` scope per manifest validation, but the registry rejects `OwnCapabilities` at `BeforePrompt` because the point has no provider context). That contradiction sits outside this PR's scope; flagging for a follow-up. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
zmanian
added a commit
that referenced
this pull request
May 23, 2026
Bundle three nit-tier review items into a single commit: **#9 Replace author-internal tags with NOTE(#3640)** The Phase-5 PR (#3640) had several `serrrfirat HIGH/MED #N on PR #3640` comment tags in this PR's diff. These are review-internal scaffolding, not load-bearing for future readers. Replaced with `NOTE(#3640)` in: - crates/ironclaw_hooks/src/dispatch.rs (self-observation guard) - crates/ironclaw_reborn/src/loop_driver_host.rs (scope validation, replay-gap milestone, subscription binding) - crates/ironclaw_reborn/tests/hooks_integration.rs (three regression tests covering scope validation, self-observation suppression, and replay-gap surfacing) - crates/ironclaw_turns/src/run_profile/host.rs (`EventSubscriptionTerminated` doc) **#10 Replace 10ms spin-poll with tokio::sync::Notify** `wait_for_seen_events` polled the shared `Mutex<Vec<SeenRuntimeEvent>>` every 10 ms until the expected count was reached. Replaced with a `SeenLog` newtype that pairs the events vec with a `Notify`; the hook's `observe()` calls `seen.push(...)` which signals `Notify::notify_one`, and `wait_for_seen_events` parks on `notified().await` under a `tokio::time::timeout`. `notify_one` is a permit-store, so an event landing between snapshot and wait still wakes the waiter immediately. Test latency drops from ~10 ms median to sub-ms and is no longer rate-limited by the polling cadence. All 30 hooks_integration tests still pass. **#11 Remove unused Clone derive on EventTriggeredHookContext** No call site clones the context — it's passed by reference. Dropped the derive to make the borrow contract clearer.
zmanian
added a commit
that referenced
this pull request
May 23, 2026
* Implement installed WASM hook runtime Adds crates/ironclaw_hooks/docs/threat-model-wasm.md and follows the reviewed design ack: 1) module bytes are resolved, digest-cached, and compiled in the tool-WASM style while reusing its resource limiter; 2) each invocation gets a fresh wasmtime Store; 3) the ABI is a wasmtime::Linker surface, not wit-bindgen; 4) host-import sink shims enforce call, patch-byte, observer-fact, and decision budgets. * Harden WASM hook string and metadata budgets * fix(hooks): validate WASM hook ABI at install time (serrrfirat #3 on PR #3634) Address serrrfirat MEDIUM finding #3: `WasmHookRuntime::prepare()` compiled and cached module bytes but did not validate imports or the requested export. ABI mismatches (unsupported import, missing export, wrong export signature) were deferred to first live dispatch — and the prior `wasm_unsupported_host_import_fails_closed` test codified that a bad-import module would install successfully and only fail closed at invocation. Malformed untrusted modules should never reach live traffic. Changes: - `prepare()` derives the target hook point from `request.kind`, then runs `validate_module_abi()`: scratch-instantiate the module against the point-specific linker (catches unsupported / wrong-type imports) and resolve the typed export `() -> ()` (catches missing export and wrong signature). Failures surface as new `WasmHookRuntimeError::InvalidImports` or existing `WasmHookRuntimeError::InvalidExport`, both of which bubble up as `HookError::RegistryConstruction` from the registrar. - `wasm_point_for_kind(HookManifestKind)` helper centralizes the kind → wasm-point mapping; the previous `execute_*` paths can share it in a follow-up but kept inline for now to minimize churn. Tests: - `wasm_unsupported_host_import_is_rejected_at_install_time`: replaces the prior test that codified late-failure behavior; asserts the registrar returns `RegistryConstruction` citing the bad import. - `wasm_missing_export_is_rejected_at_install_time`: new module that compiles but lacks the manifest-declared export; same install-time rejection. * fix(hooks): address henrypark133 must-fix #1, #2, #3 on PR #3634 Three items from the 5-15 review: **#1 (must-fix) Extract ironclaw_wasm_limiter micro-crate** Replace `#[path = "../../../ironclaw_wasm/src/limiter.rs"]` cross-crate file import with a proper Cargo edge. The 111-line `WasmResourceLimiter` moves into a new `crates/ironclaw_wasm_limiter` micro-crate that both `ironclaw_wasm` and `ironclaw_hooks` depend on. The architecture rule forbidding `ironclaw_hooks -> ironclaw_wasm` is preserved (the new crate sits below both consumers and pulls in only `wasmtime` + `tracing`); `cargo check`, `cargo doc`, and architecture-linting tests now see the edge, and the file can't be moved out from under one of the consumers silently. Mechanical changes: - new `crates/ironclaw_wasm_limiter/` (Cargo.toml + src/lib.rs with the type exposed as `pub` instead of `pub(crate)`) - workspace `members` entry added - `crates/ironclaw_wasm/src/limiter.rs` deleted - `crates/ironclaw_wasm/src/lib.rs`: `mod limiter` removed - `crates/ironclaw_wasm/src/store.rs`: import switched to `ironclaw_wasm_limiter::WasmResourceLimiter` - `crates/ironclaw_wasm/Cargo.toml`: dep added - `crates/ironclaw_hooks/Cargo.toml`: dep added - `crates/ironclaw_hooks/src/wasm/runtime.rs`: `#[path = ...]` block removed; import switched to the crate **#2 + #3 (must-fix) Dead WASM arms in dispatch** `run_before_capability_hook`, `run_before_prompt_hook`, and `run_observer_hook` each had an early-return guard that dispatched WASM hooks with `catch_unwind` + timeout, then ALSO had a matching WASM arm in the inner `match` that ran without those protections. The prompt-path arm additionally swallowed `WasmHookFailure` via `|_| ()`, making the must-fix #2 problem worse on that path specifically. If a future refactor removed any of the early-return guards, those inner arms would silently take over and drop panic isolation, deadline enforcement, AND (for prompts) the failure category. Replaced each inner arm with `unreachable!()` carrying a comment that explains why the arm exists and references the early-return guard above it. A future refactor that removes the guard will now trip the `unreachable!` at first call instead of silently degrading. All 154 hooks lib + 29 reborn integration tests still pass. * fix(hooks): plumb context to WASM hooks + runtime hardening Critical #1 on PR #3634: WASM hooks previously received no context. The `execute_*` entry points dropped the `&BeforeCapabilityHookContext` / `&BeforePromptHookContext` / `&ObserverHookContext` value and invoked the guest export with `()`, so a WASM gate could never decide based on the capability name, tenant, provider, or other dispatch-time facts. Add an `ic:hooks/context@1` host-import module exposing two read-only calls — `ctx_size() -> i32` and `ctx_read(ptr, len) -> i32` — backed by a JSON-serialized blob the dispatcher writes per-invocation into the fresh store. Modules that don't import these continue to link; modules that do import them get a stable, non-empty payload to read. An integration test (`wasm_before_capability_hook_reads_context_blob`) asserts the contract end-to-end: a guest that fails to read a non-empty blob traps before its `deny` call. Also rolls up the other reviewer-flagged WASM runtime issues, all of which touch `wasm/runtime.rs`: HIGH #2: epoch-tick background thread now holds a shutdown `AtomicBool` and joins on `Drop`. Previously it looped forever and leaked an Engine clone on every runtime drop. MED #4: compiled-module cache is now an `lru::LruCache` bounded by `MODULE_CACHE_CAPACITY = 128`. Replaces the unbounded `HashMap`. MED #7: `prepare()` no longer compiles under the cache lock. Fast path reads from LRU under a brief lock; slow path compiles outside the lock and re-checks on insert to avoid the TOCTOU window where two concurrent installs of the same module both compile. Bug #9: post-call `deadline_exceeded()` re-check on the Ok branch is gone. wasmtime epoch-interrupt is the authoritative wall-clock signal; an Ok return is no longer reclassified as a timeout because the wall ticked over during host-side return. Bug #10: `add_milestone_metadata` returns a distinct "metadata value exceeds the u32 byte-length ceiling" error when the guest-supplied `value.len()` overflows u32, instead of misreporting it as "exceeded total prompt-patch byte budget". Existing integration tests for WASM hooks are also re-wired through `HookRegistrar::with_verified_grants` so the grants-store gate added in the foundation-01 merge stops failing the pre-existing fixtures. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(hooks): run WASM hooks on the blocking pool HIGH #3 on PR #3634: `tokio::time::timeout` does NOT cancel synchronous wasmtime execution. The previous code awaited a `catch_unwind(async { h.evaluate(ctx) })` future whose body completed in one poll, so the timeout could only fire *around* the WASM call rather than against it; a hook that wedged inside wasmtime simply pinned the calling tokio task. Route gate, prompt, and observer WASM dispatch paths through `tokio::task::spawn_blocking` via a shared `run_wasm_blocking` helper. The outer `tokio::time::timeout` now governs the JoinHandle, so a stuck blocking task stops blocking the dispatcher's caller; the wasmtime epoch interrupt configured in the runtime (10 ms tick) is the authoritative in-WASM wall-clock cancel signal. JoinError (panic in the blocking task) maps to `FailureCategory::Panic`, matching the pre-existing semantics for synchronous panics caught via `catch_unwind`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * perf(hooks): O(1) hook-id lookup via side index Finding #8 on PR #3634: `set_priority`, `poison`, `is_poisoned`, and `contains_hook` all did full-registry scans over every binding at every point. Each is called per-dispatch (poison-checks on the snapshot loop in particular), so the cost is `O(registered_hooks)` per `(installed_hook, registered_hook)` pair. Maintain a denormalized `HashMap<HookId, (HookPointSpec, usize)>` side index in lock-step with `by_point` so every per-hook-id operation becomes a single hash lookup + a direct vec indexed access. The duplicate-id rejection in `insert` now reads from the side index too, turning what used to be a flat-map scan into a `HashMap::contains_key`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test(hooks): wall-clock timeout, observer memory, limiter rollback, registrar happy path Round out the test set for the WASM hook execution path: #11 / #12: gate + observer wall-clock timeout. The pre-fix dispatcher ran wasmtime synchronously on the executor, so the outer `tokio::time::timeout` `Err(_elapsed)` arm was effectively unreachable. Now that WASM execution runs on the blocking pool, the timeout actually fires; the new tests give the wasm budget headroom (1B fuel, 5s wall) and the dispatcher a 20 ms timeout, then assert the failure classification (FailClosed for gate, FailIsolated for observer). #13: observer memory exhaustion. Mirrors `wasm_memory_exhaustion_fails_closed_for_gate` against the observer dispatch path so the FailIsolated branch of the failure matrix has explicit memory coverage, not just fuel/wall. #15: `WasmResourceLimiter::memory_grow_failed` rollback. Stages an approved grow, simulates the OS-level grow failing, and asserts a subsequent grow of the full ceiling succeeds — the inflated `memory_used` from the failed attempt must be released. #16: registrar WASM happy path. Companion to the existing `install_wasm_body_requires_runtime` negative case: a valid module installs, the binding is visible via the public registry accessor, and is not pre-poisoned. #14 (`add_milestone_metadata` happy path) is intentionally omitted — the BeforePrompt dispatch path is currently unreachable due to a pre-existing manifest-vs-registry scope conflict (`OwnCapabilities` is the only valid `BeforePrompt` scope per manifest validation, but the registry rejects `OwnCapabilities` at `BeforePrompt` because the point has no provider context). That contradiction sits outside this PR's scope; flagging for a follow-up. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * refactor(hooks): typed WASM version material, reconcile design doc LOW #20 on PR #3634: extract the `{extension_version}+wasm:{module_digest_hex}` concatenation into a `WasmVersionMaterial` newtype with a single `Display` impl. The identity material no longer floats free as a stringly-typed argument inside the registrar. Reconcile `docs/successors/02-wasm-runtime.md` with the implementation: - Spell out that wall-clock cancellation depends on the `tokio::time::timeout(tokio::task::spawn_blocking(...))` pair, and explain why a bare timeout over a synchronous wasmtime call cannot actually cancel. - Define `FailIsolated` and `FailClosed` as `FailureDisposition` values, distinct from the older `HookFailureMode::{FailOpen, FailClosed}` policy switch that applies to predicates. - Clarify the generic `evaluate` export contract — name is whatever the manifest declares, signature is `(): ()`, context arrives through the new `ic:hooks/context@1` host imports — and note the intentional divergence from `WitToolRuntime`'s hardcoded interface. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(hooks): drop .expect() in WASM module cache capacity Pre-commit no-panics CI flagged the .expect() on the LruCache capacity. Move the validity check to a const match, so the NonZeroUsize is fixed at compile time and the no-panics regex is satisfied. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(hooks): use HookLocalId::new after newtype privatization The newtype-privatization landed in reborn-integration after the hooks-fu-wasm-runtime branch's WASM scaffolding tests were written; update the affected test/registrar sites to use HookLocalId::new instead of the now-private tuple constructor. * style: cargo fmt after newtype-privatization fixups * test(hooks): ignore 3 BeforePrompt WASM tests with manifest/registry conflict These tests were failing on the original branch tip too (verified against origin/hooks-fu-wasm-runtime @ 571efdf). The Installed-tier BeforePrompt WASM install path has no valid scope today: - OwnCapabilities is rejected by the registry C3 check (finding #2 on PR #3573) since BeforePrompt has no per-capability invocation context. - SameTenant is rejected by manifest validation ("cannot combine scope = same_tenant with kind = before_prompt"). The budget-overflow paths these tests exercise are point-agnostic; the follow-up is to either rewrite the helper to install through BeforeCapability or add a Global manifest scope. Tracked as a deferred item on the new PR. --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
zmanian
added a commit
that referenced
this pull request
May 23, 2026
Bundle three nit-tier review items into a single commit: **#9 Replace author-internal tags with NOTE(#3640)** The Phase-5 PR (#3640) had several `serrrfirat HIGH/MED #N on PR #3640` comment tags in this PR's diff. These are review-internal scaffolding, not load-bearing for future readers. Replaced with `NOTE(#3640)` in: - crates/ironclaw_hooks/src/dispatch.rs (self-observation guard) - crates/ironclaw_reborn/src/loop_driver_host.rs (scope validation, replay-gap milestone, subscription binding) - crates/ironclaw_reborn/tests/hooks_integration.rs (three regression tests covering scope validation, self-observation suppression, and replay-gap surfacing) - crates/ironclaw_turns/src/run_profile/host.rs (`EventSubscriptionTerminated` doc) **#10 Replace 10ms spin-poll with tokio::sync::Notify** `wait_for_seen_events` polled the shared `Mutex<Vec<SeenRuntimeEvent>>` every 10 ms until the expected count was reached. Replaced with a `SeenLog` newtype that pairs the events vec with a `Notify`; the hook's `observe()` calls `seen.push(...)` which signals `Notify::notify_one`, and `wait_for_seen_events` parks on `notified().await` under a `tokio::time::timeout`. `notify_one` is a permit-store, so an event landing between snapshot and wait still wakes the waiter immediately. Test latency drops from ~10 ms median to sub-ms and is no longer rate-limited by the polling cadence. All 30 hooks_integration tests still pass. **#11 Remove unused Clone derive on EventTriggeredHookContext** No call site clones the context — it's passed by reference. Dropped the derive to make the borrow contract clearer.
zmanian
added a commit
that referenced
this pull request
May 23, 2026
Bundle three nit-tier review items into a single commit: **#9 Replace author-internal tags with NOTE(#3640)** The Phase-5 PR (#3640) had several `serrrfirat HIGH/MED #N on PR #3640` comment tags in this PR's diff. These are review-internal scaffolding, not load-bearing for future readers. Replaced with `NOTE(#3640)` in: - crates/ironclaw_hooks/src/dispatch.rs (self-observation guard) - crates/ironclaw_reborn/src/loop_driver_host.rs (scope validation, replay-gap milestone, subscription binding) - crates/ironclaw_reborn/tests/hooks_integration.rs (three regression tests covering scope validation, self-observation suppression, and replay-gap surfacing) - crates/ironclaw_turns/src/run_profile/host.rs (`EventSubscriptionTerminated` doc) **#10 Replace 10ms spin-poll with tokio::sync::Notify** `wait_for_seen_events` polled the shared `Mutex<Vec<SeenRuntimeEvent>>` every 10 ms until the expected count was reached. Replaced with a `SeenLog` newtype that pairs the events vec with a `Notify`; the hook's `observe()` calls `seen.push(...)` which signals `Notify::notify_one`, and `wait_for_seen_events` parks on `notified().await` under a `tokio::time::timeout`. `notify_one` is a permit-store, so an event landing between snapshot and wait still wakes the waiter immediately. Test latency drops from ~10 ms median to sub-ms and is no longer rate-limited by the polling cadence. All 30 hooks_integration tests still pass. **#11 Remove unused Clone derive on EventTriggeredHookContext** No call site clones the context — it's passed by reference. Dropped the derive to make the borrow contract clearer.
zmanian
added a commit
that referenced
this pull request
May 23, 2026
* docs(hooks): scope event-triggered hooks (Phase 5, successor #4) Successor PR from #3573. Adds a new EventTriggered hook point that subscribes to RuntimeEvents asynchronously, outside the loop's inline tick. Observer-only by construction (no Allow/Deny/Patch); typed against a narrowed HookObservableEvent projection to keep the cross-crate boundary clean. Scope doc only; design questions about cursor/replay semantics and per-extension event-rate caps need design review before implementation. * Implement Phase 5 event-triggered hooks Cites crates/ironclaw_hooks/docs/successors/04-event-triggered-hooks.md as the scope contract. Adds the EventTriggered observer hook point, durable RuntimeEvent dispatch path, and Reborn pull-driven subscription wiring with caller-level coverage for matching, replay, scope filtering, observer-only authority, and backpressure. * Fix hook event OwnCapabilities owner lookup * fix(hooks): carry owning extension into hook milestone runtime events henrypark133 HIGH + codex P1 on PR #3640: `OwnCapabilities`-scoped event-triggered subscriptions silently never fired for `HookFailed`/`HookDecisionEmitted`/`HookDispatched` events because those `RuntimeEvent` constructors hardcoded `provider: None`. Since Installed hooks default to `OwnCapabilities`, the very events that Phase 5 was designed to observe (hook-failure / decision alerting) never reached their default-configured subscriber. A prior fix added a hook_id-based fallback in `scope_provider_for_runtime_event` that resolves the owning extension through the registry's hex index when `event.provider` is `None`. That covers the case where the failing hook is still registered at replay time, but the durable fix is to stamp the originating provider into the event at emit time so the primary `event.provider` path resolves without any fallback. Plumbed `owning_extension: Option<ExtensionId>` end-to-end: - `LoopHostMilestoneKind::{HookDispatched, HookDecisionEmitted, HookFailed}` gain the field (with `#[serde(default, skip_serializing_if = "Option::is_none")]` so pre-existing checkpoint payloads and the L3 schema-snapshot tests round-trip unchanged when no owner is set). - `RuntimeEvent::hook_{dispatched, decision_emitted, failed}` constructors accept the owner and stamp it into `provider`. - `milestone_events.rs` threads the field through the projection. - `HookDispatcher::emit_dispatched/emit_decision` pass `binding.owning_extension.clone()` directly. - `HookDispatcher::emit_failure` (no binding handy on the failure path) looks the owner up via the registry's existing `owning_extension_for_hook_hex` index. Tests: - `event_triggered_own_capabilities_matches_hook_failed_with_carried_provider`: primary-path regression — two `HookFailed` events with `provider: Some(ext_a|ext_b)` against an `OwnCapabilities` subscription scoped to ext_a; only the own-provider event fires and `event.provider == Some(ext_a)`. - Existing `event_triggered_own_capabilities_scope_resolves_hook_failed_owner_from_hook_id` remains green: passes `None` for the new arg so the fallback path is still exercised for legacy payloads. All other call sites updated to pass `None` (no owner available) or the resolved owner where applicable. * fix(hooks): validate event subscription scope against run scope (serrrfirat HIGH #1 on PR #3640) `EventTriggeredHookSubscription` accepted a caller-supplied `EventStreamKey` + `ReadScope` and used `run_context.scope.tenant_id` as the hook context's tenant — with no validation that the two agreed. A caller wiring tenant A's host with tenant B's stream would cause hooks to observe B's events while the hook context claimed tenant A. Cross-tenant trust-boundary break. Add `EventTriggeredHookSubscription::validate_against_run_scope` and call it from `build_text_only_host_with_capabilities` before spawning. Validation: - Stream `(tenant_id, user_id, agent_id)` must equal `(run_context.scope.tenant_id, thread_scope.owner_user_id, run_context.scope.agent_id)`. - Thread without `owner_user_id` cannot bind any subscription — the user dimension is required to verify stream identity. - Every `Some(want)` in `ReadScope` must equal the corresponding run/thread scope value (project/mission/thread). `None` is permissive (run scope owns the dimension authoritatively). Failures surface as `RebornLoopDriverHostError::ScopeMismatch` with a specific reason naming the offending dimension. Tests: - `event_triggered_subscription_with_foreign_tenant_stream_fails_host_build` - `event_triggered_subscription_with_foreign_user_stream_fails_host_build` The integration fixture's `ThreadScope` now sets `owner_user_id: Some(...)` so it passes validation; previously it was `None`, which the new check (correctly) refuses. Existing tests continue to pass. * fix(hooks): surface event subscription replay-gap as a milestone (serrrfirat MED on PR #3640) When the durable event log returned `EventError::ReplayGap`, the event-triggered subscription's background task previously logged a `tracing::warn!` and broke out of the poll loop — silently killing all future hook event delivery for the run with no operator-visible signal. A scoped audit hook that mattered to compliance would just stop, and nobody downstream would know. Surface the termination through the host's milestone sink: - New `LoopDriverNoteKind::EventSubscriptionTerminated` variant. - The subscription's `spawn`/`run` now takes the host's `Arc<dyn LoopHostMilestoneSink>` and the active `LoopRunContext`. On `ReplayGap`, it constructs a `DriverNote` milestone with that kind plus a `LoopSafeSummary` describing the gap, publishes it through the same sink that carries every other host milestone, and *then* breaks (fail-closed: the at-most-once contract is already broken; resuming from `earliest` would silently lose the gap). - Log level bumped from `warn` to `error` to match the severity. - A best-effort send: failures to publish the milestone are logged but do not stall the subscription teardown. Tests: - `event_triggered_replay_gap_emits_subscription_terminated_milestone`: appends 3 events, `truncate_before_or_at` to cursor 2 to force a replay gap, starts the subscription from cursor origin (now stale), and asserts a `DriverNote { kind: EventSubscriptionTerminated, .. }` shows up on the host's milestone sink within a 2s deadline. Self-emit reentrancy (serrrfirat MED #3 on the same PR) is intentionally not addressed here — that fix needs a design call (task-local re-entry flag vs. removing RuntimeEvent emit capability from event-hook execution contexts) and is a follow-up. * fix(hooks): suppress event-triggered self-observation (serrrfirat MED #3 on PR #3640) A hook that subscribes to one of the hook-lifecycle event kinds (`HookDispatched`/`HookDecisionEmitted`/`HookFailed`) with a scope that matches its own provider would otherwise be dispatched for events describing its OWN executions. The dispatcher emits those events itself when running the hook, so a hook subscribing to `HookFailed` with `OwnCapabilities` against its own extension would fail → emit HookFailed → re-dispatch → fail → emit → … storm. `dispatch_event_triggered_at` now skips events whose `event.hook_id` equals the binding's own hook id when the event kind is a hook- lifecycle kind (`is_hook_lifecycle_kind`). The check is intentionally narrow: - It only fires for hook-lifecycle events. Subscriptions to other event kinds are unaffected. - It only suppresses literal self-observation; events about other hooks (even hooks from the same extension) still dispatch. This does NOT cover the broader case of a hook that captures an `Arc<DurableEventLog>` and mints arbitrary `RuntimeEvent`s from inside its `observe()`. That requires architectural restriction on what hook impls can capture — tracked separately as a follow-up. Tests: - `event_triggered_self_lifecycle_event_does_not_redispatch`: appends two `HookFailed` events with the same provider — one targeting the subscriber's own hook id, one targeting a different hook. Asserts only the OTHER hook's failure fires (proves the filter is narrow, not blanket). * fix(hooks): address henrypark133 should-fix #1, #2, #3, #6 on PR #3640 Four items from the 5-15 review (#4 DoS budget and #5 narrowed projection deferred — see below): **#1 (should-fix) Invariant: EventTriggered ↔ event_kind_filter** `HookRegistry::insert` now enforces the biconditional at install time: an `EventTriggered` binding must declare an `event_kind_filter` (otherwise the dispatcher's kind match would silently never fire — a no-op binding), and conversely only `EventTriggered` bindings may declare a filter (other points are kind-agnostic and would ignore the field). Misconfigured bindings fail loud at install. **#2 (should-fix) Remove `Clone` derive on EventTriggeredHookSubscription** `Clone` on a spawn-semantics type was a footgun: external callers cloning + spawning twice would create two consumers reading from the same `start_cursor`, each dispatching every hook. Replace with an explicit `clone_for_independent_spawn(&self)` method named verbosely so the property is visible at the seam. Internal use updated in the factory's host-build path; external callers can no longer accidentally construct a dual-consumer pattern. **#3 (should-fix) catch_unwind around the background `run()` task** The subscription's tokio task body now runs inside `AssertUnwindSafe(...).catch_unwind()`; a panic in `run()` emits the same `EventSubscriptionTerminated` `DriverNote` milestone the `ReplayGap` path already emits, instead of silently terminating with no operator-visible signal. **#6 (should-fix) Replay semantics in rustdoc on public API** Added a "Replay semantics" section to `EventTriggeredHookSubscription` rustdoc: at-least-once, caller-owned cursor persistence, the restart-from-start_cursor replay pattern. Previously only in the design doc; now load-bearing API contract is visible at the type. **#4 (deferred) Per-hook DoS budget for Installed tier** Henry's recommendation was to gate `Installed`-tier event-triggered hooks entirely until the budget design lands, allowing only Builtin/Trusted. That breaks 11 existing tests + the primary use case. Instead: documented the existing first-line throttle (`batch_limit` × `poll_interval`) as the current bound on indirect- recursion fanout, and tracked the full per-hook rate cap with poisoning + milestone-on-overrun as a follow-up. The self-trigger guard (committed earlier in this PR) catches the most common direct pattern; the throttle here bounds the indirect pattern until the proper budget lands. **#5 (deferred) Narrowed `HookObservableEvent` projection** Would prevent full `RuntimeEvent` surface from reaching Installed- tier hooks. Project-wide impact (events crate types, projection glue). Tracked as a follow-up; the existing sanitized-event projection bounds the surface to closed-vocab labels. All 156 hooks lib + 30 reborn integration tests pass. * chore(hooks): address nits from PR #3640 review Bundle three nit-tier review items into a single commit: **#9 Replace author-internal tags with NOTE(#3640)** The Phase-5 PR (#3640) had several `serrrfirat HIGH/MED #N on PR #3640` comment tags in this PR's diff. These are review-internal scaffolding, not load-bearing for future readers. Replaced with `NOTE(#3640)` in: - crates/ironclaw_hooks/src/dispatch.rs (self-observation guard) - crates/ironclaw_reborn/src/loop_driver_host.rs (scope validation, replay-gap milestone, subscription binding) - crates/ironclaw_reborn/tests/hooks_integration.rs (three regression tests covering scope validation, self-observation suppression, and replay-gap surfacing) - crates/ironclaw_turns/src/run_profile/host.rs (`EventSubscriptionTerminated` doc) **#10 Replace 10ms spin-poll with tokio::sync::Notify** `wait_for_seen_events` polled the shared `Mutex<Vec<SeenRuntimeEvent>>` every 10 ms until the expected count was reached. Replaced with a `SeenLog` newtype that pairs the events vec with a `Notify`; the hook's `observe()` calls `seen.push(...)` which signals `Notify::notify_one`, and `wait_for_seen_events` parks on `notified().await` under a `tokio::time::timeout`. `notify_one` is a permit-store, so an event landing between snapshot and wait still wakes the waiter immediately. Test latency drops from ~10 ms median to sub-ms and is no longer rate-limited by the polling cadence. All 30 hooks_integration tests still pass. **#11 Remove unused Clone derive on EventTriggeredHookContext** No call site clones the context — it's passed by reference. Dropped the derive to make the borrow contract clearer. * docs(hooks): reconcile event-triggered hooks design doc with Phase 5 reality Address gemini-code-assist review on `04-event-triggered-hooks.md`: - L50 (Likely surface): annotated the sketch's full `RuntimeEvent` use with a pointer to the narrowed-projection follow-up so the snippet no longer reads as a recommendation contradicting L119–121. - L55 (sink methods): replaced `note_fact` / `emit_audit` (which never shipped on `ObserverSink`) with the actual `note(category, summary)` primitive and cross-referenced Reborn's `EventTriggeredObserverSink`. - L95 (cursor / replay): "lost events during downtime acceptable" contradicted the at-least-once replay semantics described in the Phase 5 implementation notes. Rewrote the bullet to say replay is at-least-once from the persisted cursor and to spell out the operator obligation around cursor persistence before shutdown. - L100/115 (forbids events dep): the original doc claimed `ironclaw_hooks` forbids an `ironclaw_events` dep, but the Risk section noted the dep is already established via PR #3573. Updated both passages to reflect that the dep direction is set; Phase 5 adds the *consumer* side. The narrowed `HookObservableEvent` projection is now framed as a follow-up tracked in #3690. * refactor(hooks): unify event-triggered sink with ObserverSink Address PR #3640 review findings A3, C4, F14, and cluster G: - F14: drop duplicate `EventTriggeredObserverSink` trait and reuse `ObserverSink` directly in the `EventTriggeredHook` trait. The two surfaces were signature-identical; keeping them separate let them drift, and a future gate/mutator method added to one would not surface as a compile error on the other. - A3: add `is_replay: bool` to `EventTriggeredHookContext` and a dedicated `dispatch_event_triggered_replay_at` entry point. The subscription contract is at-least-once, so side-effecting hooks need to dedupe by `event.event_id` on restart-driven replay. - C4: index event-triggered bindings by `RuntimeEventKind` at install time so dispatch is O(matches) instead of scanning every event-triggered binding for every event. - Cluster G: doc/04-event-triggered-hooks.md updated to reflect the unified sink, the explicit at-least-once semantics + `is_replay` signal, the actual `note(category, summary)` primitive (not the speculative `note_fact` / `emit_audit`), the corrected `ironclaw_events` dep status, and the issue #3690 reference for the narrowed `HookObservableEvent` projection. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * perf(hooks): adaptive backoff for event-triggered subscription Address PR #3640 review findings C5, A1, A2: - C5: empty-poll backoff for `EventTriggeredHookSubscription`. The previous loop hammered the durable log at a fixed 50ms cadence under sustained idle, even when no events had arrived for minutes. The subscription now tracks consecutive empty polls and sleeps for `min(poll_interval << streak, max_poll_interval)` before the next poll, defaulting to a 1s cap; a non-empty batch resets the streak so producer bursts restore low-latency dispatch immediately. Exposed via `with_max_poll_interval` so callers can tune. - A1 / A2: explicit issue references for the deferred narrowed `HookObservableEvent` projection (#3690) and the per-hook DoS dispatch budget (#3689). The current self-trigger guard catches direct-recursion storms; the backoff bounds indirect ones until the proper budget design lands. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test(hooks): cover event-triggered dispatch edge cases Address PR #3640 review findings D8, D9, D10, D11, D12, and B: - D8: dispatching an event-triggered binding that has no installed hook impl must poison the slot and surface a Malformed failure rather than silently no-op. A follow-up dispatch on the same kind must skip the poisoned slot. - D9: registry validation rejects non-event-point bindings that carry an `event_kind_filter`, mirroring the existing reverse-direction check. - D10: the existing hook-meta serde round-trip tests always passed `None` for `owning_extension` and never asserted `event.provider`. Add `hook_meta_events_round_trip_owning_extension_as_provider` to pin the projection that scope filtering depends on. - D11: `scope_provider_for_runtime_event` falls back to `None` when the registry mutex is poisoned. Force a poison on a spawned thread and assert the resolver remains fail-closed. - D12: `run_event_triggered_hook` catches panics from the hook impl via `AssertUnwindSafe::catch_unwind`. Drive it with a deliberately panicking impl and assert `FailureCategory::Panic`. - Cluster B: when a hook-meta event has `provider: None`, the dispatcher recovers the owning extension from the registry's hex-keyed index so `OwnCapabilities` watchers still fire. Add a full end-to-end test exercising that path through `dispatch_event_triggered_at`. Also pin C4 indexing: a registry-level test that `active_for_event_kind` returns only bindings whose declared filter matches. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(hooks): port event-triggered tests after foundation rebases (#3911/#3912/#3913) - Add event_kind_filter: None to HookBinding test constructions (foundation added new field) - Replace tuple-struct construction of ExtensionId/HookLocalId with ::new() per #3912 newtype privatization - Lowercase RuntimeEventKind debug repr for HookLocalId validation (lowercase-only identifiers) - Replace pub-use re-exports with module-path imports per foundation cleanup - Rename fixture.user_id to fixture.actor_id per #3633 final naming * fix(hooks): adapt event-triggered to WASM hook runtime (#3920) - Add event_kind_filter: None to WASM HookBinding constructions in dispatch.rs - Extend HookManifestKind match arms in registrar.rs and wasm/runtime.rs to handle EventTriggered (rejected: WASM-bodied event-triggered hooks are not yet supported) --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This was referenced Jun 3, 2026
pranavraja99
pushed a commit
that referenced
this pull request
Jun 12, 2026
Resolves Henry + Firat review comments on #4588: - Composition-owned RebornTrajectoryObserver trait + adapter to the loop-support CapabilityTrajectoryObserver, instead of re-exporting the substrate trait directly (CLAUDE.md: facade-shaped handles only). Loop-support contract changes no longer break the public Reborn API. (Henry#8) - Safe-preview by default: with_trajectory_observer now forwards bounded (truncated strings / capped arrays) payloads so a logs/UI/telemetry sink stays within the model-visible display boundary; a trusted in-process consumer that needs verbatim tool I/O opts in via the new with_raw_trajectory_observer. (Henry#5) - catch_unwind around both observer call sites (input hook in capability_port, result hook in LocalDevCapabilityIo) so a panicking observer can't unwind the capability hot path; trait doc now states the never-block / panic-caught contract. (Henry#1/#6) - e2e test local_dev_runtime_forwards_tool_call_trajectory_to_raw_observer: drives a real build_reborn_runtime turn dispatching builtin.echo and asserts BOTH input and result callbacks fire on the genuine dispatch path — honest coverage that replaces the dropped direct-call result-hook test, and proves the observer threads through build_reborn_runtime. (Firat#1, Henry#3/#7) - Strengthened provider-injection docs: the config-vs-override invariant and why the feature-gated seam takes the LlmProvider substrate trait. (Henry#4/#9/#11) - Fixed the LocalDevCapabilityIo observer field comment to describe its actual result-only responsibility. (Henry#10) Provider-override coverage (Firat#2/Henry#2) already landed in build_llm_gateway_drives_provider_override_not_config. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
zmanian
pushed a commit
to zmanian/ironclaw
that referenced
this pull request
Jun 15, 2026
…r injection (nearai#4588) * feat(reborn): expose a trajectory observer hook on RebornRuntimeInput The reborn runtime is sealed: build_reborn_runtime returns only the final AssistantReply, and per-step capability (tool) calls + results live in internal stores. Downstream consumers (benchmark harnesses, UI/debuggers) can't observe the agent's trajectory. Add `RebornTrajectoryObserver` (pub trait: on_capability_input(call_id, name, args) / on_capability_result(call_id, output)) and `RebornRuntimeInput::with_trajectory_observer`. The local-dev capability IO (`LocalDevCapabilityIo`) forwards each tool call's name+args (at input staging) and result (at result write) to the observer when present — reusing the same data it already records for display previews. No-op when unset; best-effort. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * debug: trace observer hook firing (temporary) * feat(reborn): trajectory observer — capability_id on result, reliable spine Provider tool calls are staged by a lower decorator that bypasses the LocalDevCapabilityIo input path, so on_capability_input does not fire for them. on_capability_result fires for every completed capability — make it carry the capability_id so consumers can reconstruct the trajectory (name + output) from results alone. Input args capture is a follow-up. * feat(reborn): capture capability input args at the host port chokepoint Provider tool calls are staged by ProviderToolCallInputResolver, which keeps args in a private map and bypasses the capability-IO input hook — so inputs never reached the trajectory observer (only results did). Move the observer trait down to ironclaw_loop_support (CapabilityTrajectoryObserver, re-exported from composition as RebornTrajectoryObserver) and hook it in HostRuntimeLoopCapabilityPort::invoke_capability right after the input resolves — the one place the model's resolved arguments are visible. Threaded through HostRuntimeLoopCapabilityPortFactory + the local-dev factory. Result hook unchanged. Now name + args + output are all captured. * feat(reborn): host LLM-provider injection seam ResolvedRebornLlm::with_provider — drive the runtime with a caller-supplied LlmProvider (e.g. an instrumented wrapper that counts tokens/cost and captures reasoning) instead of always building one from config; build_llm_gateway honors the override. The only viable observability path for reborn, whose model calls run in spawned worker tasks a per-task tracing subscriber can't reach. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(reborn): cover trajectory observer + LLM provider override seams Addresses Firat's two blocking review findings on nearai#4588 (both missing-integration-test, per AGENTS.md "test through the caller"): 1. Trajectory observer callbacks — drive the real call sites with a recording CapabilityTrajectoryObserver: - host port: invoke_capability via HostRuntimeLoopCapabilityPortFactory ::with_trajectory_observer asserts on_capability_input fires with the resolved capability id + tool-call arguments. - local-dev IO: register_provider_tool_call_input + write_capability_result assert on_capability_input and on_capability_result fire and correlate by input ref. 2. LLM provider override — build_llm_gateway_drives_provider_override_not_config injects a counting mock via ResolvedRebornLlm::with_provider, points config at a dead endpoint, and asserts the gateway returns the mock's sentinel (proving the override is driven, not a config-built chain). Also fixes pre-existing breakage this surfaced: 5 LocalDevLoopCapabilityPort Factory test initializers (shell_tests.rs + tests.rs) were missing the trajectory_observer field added by this PR, so the composition crate's tests did not compile under --features root-llm-provider. loop_support: 301 passed; composition (root-llm-provider): 520 passed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(reborn): make trajectory observer input semantics consistent Addresses Copilot's follow-up findings on the observer seam: - Drop the `on_capability_input` callback from `LocalDevCapabilityIo:: register_provider_tool_call_input`. It forwarded the raw provider tool name (`builtin_echo`) as the capability id — conflicting with the observer contract (resolved dotted `builtin.echo`) and the authoritative port-level hook — and `ProviderToolCallInputResolver` doesn't delegate here for provider tool calls, so it never fired in practice anyway. `HostRuntimeLoopCapabilityPort::invoke_capability` remains the single source of `on_capability_input` (resolved id); `LocalDevCapabilityIo` remains the source of `on_capability_result`. - Clarify the trait doc: `arguments` is the raw model-emitted tool-call input resolved from the input ref (the callback fires before schema normalization), which is what the trajectory should record. - Refocus the local-dev test on `on_capability_result` forwarding + correlation, and assert input staging does NOT emit `on_capability_input` from local-dev IO. Port-level input semantics stay covered by the capability_port.rs test. loop_support: 301 passed; composition (root-llm-provider): 520 passed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * wire trajectory_observer through RefreshingLocalDevCapabilityPortConfig Completes the main-merge conflict resolution: local_dev.rs passes trajectory_observer into the refreshing-port config, so the config struct + port struct must carry it and build_inner must apply it via .with_trajectory_observer(). (Missed staging this file in the merge commit.) * test(reborn): lock down the observability seams against regression nearai#4588 exposes two seams a downstream harness relies on. Add tests so a future refactor can't silently break either: - capability_io_forwards_result_to_trajectory_observer: drives write_capability_result and asserts on_capability_result fires with the correct (call_id, capability_id, output) — the result half of the trajectory observer (tool-call outputs). - build_llm_gateway_drives_provider_override_not_config: asserts the gateway drives a provider injected via ResolvedRebornLlm::with_provider (config points at a dead endpoint), proving the provider-injection seam works — this is how the bench captures reasoning / tokens / cost / system-prompt / tool-definitions. (Restores the test dropped during the main merge.) The input half (on_capability_input) is already covered by invoke_capability_forwards_resolved_input_to_trajectory_observer in ironclaw_loop_support. All three pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(reborn): drop the false-confidence result-hook test capability_io_forwards_result_to_trajectory_observer called write_capability_result directly, so it stayed green even though the result hook is unreachable end-to-end while capability dispatch fails (the LocalDevYolo InputEncode regression) — i.e. it did not fail when the feature it claimed to cover was actually broken. Remove it rather than ship false confidence. The result hook lives in LocalDevCapabilityIo and is only reached by a real local-dev runtime turn, so an honest guard must drive the full runtime and is red until the dispatch regression is fixed; that guard belongs as an end-to-end test (PR, once green) or a bench pre-flight, not a direct-call unit test. Kept: invoke_capability_forwards_resolved_input_to_trajectory_observer (input hook, real port code path) and build_llm_gateway_drives_provider_override_not_config (provider seam) — both genuinely fail if their seam regresses. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(reborn): address review on the trajectory-observer + provider seams Resolves Henry + Firat review comments on nearai#4588: - Composition-owned RebornTrajectoryObserver trait + adapter to the loop-support CapabilityTrajectoryObserver, instead of re-exporting the substrate trait directly (CLAUDE.md: facade-shaped handles only). Loop-support contract changes no longer break the public Reborn API. (Henry#8) - Safe-preview by default: with_trajectory_observer now forwards bounded (truncated strings / capped arrays) payloads so a logs/UI/telemetry sink stays within the model-visible display boundary; a trusted in-process consumer that needs verbatim tool I/O opts in via the new with_raw_trajectory_observer. (Henry#5) - catch_unwind around both observer call sites (input hook in capability_port, result hook in LocalDevCapabilityIo) so a panicking observer can't unwind the capability hot path; trait doc now states the never-block / panic-caught contract. (Henry#1/nearai#6) - e2e test local_dev_runtime_forwards_tool_call_trajectory_to_raw_observer: drives a real build_reborn_runtime turn dispatching builtin.echo and asserts BOTH input and result callbacks fire on the genuine dispatch path — honest coverage that replaces the dropped direct-call result-hook test, and proves the observer threads through build_reborn_runtime. (Firat#1, Henry#3/nearai#7) - Strengthened provider-injection docs: the config-vs-override invariant and why the feature-gated seam takes the LlmProvider substrate trait. (Henry#4/nearai#9/nearai#11) - Fixed the LocalDevCapabilityIo observer field comment to describe its actual result-only responsibility. (Henry#10) Provider-override coverage (Firat#2/Henry#2) already landed in build_llm_gateway_drives_provider_override_not_config. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(reborn): second-round review fixes on the trajectory/provider seams Addresses Henry's review of the first round (nearai#4588): - safe_preview_value now bounds objects (entry cap), recursion depth, and total nodes — not just strings/arrays — so a wide or deeply nested capability result can't force unbounded traversal/allocation on the hot path. (3405419089) - Narrowed the loop-support CapabilityTrajectoryObserver to input-only: HostRuntimeLoopCapabilityPort never staged results through the port (results go via LoopCapabilityResultWriter), so advertising on_capability_result there was a contract a direct user could never see fire. Result observation stays on the composition path (LocalDevCapabilityIo). (3405419104) - Synthetic capabilities (e.g. builtin.skill_activate) bypass the inner port's input hook, so the synthetic wrapper now emits on_capability_input itself after resolving input — otherwise consumers saw an unpaired result with no args. (3405419110) - Provider injection no longer accepts a wholesale Arc<dyn LlmProvider> through the facade: with_provider is replaced by with_provider_factory, a decorator Fn(Arc<dyn LlmProvider>) -> Arc<dyn LlmProvider>. The composition always builds the provider from config (config stays the single construction source — collapses the old config-vs-override invariant too) and hands it to the factory to wrap. (3405419100, 3405419146) - New caller-level test local_dev_runtime_safe_preview_observer_receives_bounded_payload: installs the default with_trajectory_observer, drives a real turn with a large echo payload, asserts the observer receives a truncated preview. (3405419095) - Dropped the stale nearai#4588/main-rebase comment for a durable invariant. (3405419113) cargo test (loop_support + reborn_composition, single-threaded) green; clippy clean on touched files. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * style(reborn): rustfmt the trajectory/provider review changes Formatting-only: import grouping + mod ordering in the two lib.rs re-export blocks, and wrapping in runtime.rs / local_dev.rs / trajectory_observer.rs. Fixes the Formatting + Code Style CI checks. No behaviour change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * style(reborn): drop std-Mutex guard before await in observer e2e tests clippy::await_holding_lock (-D warnings): the two trajectory-observer e2e tests held the observer's std::sync::Mutex guard across runtime.shutdown().await. Shut down before inspecting the recorded callbacks (the data is already captured during the turn) so no guard is held across an await. Fixes Clippy (all-features). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * WIP(bench): http empty-body + multi-tool-call port reuse + final-answer nudge Local checkpoint so the bench builds against a stable tree (uncommitted edits were being reverted mid-session). Bundles: http body() empty-field fix, RefreshingLocalDevCapabilityPort register reuse, the gated final-answer nudge + interactive_profile gate flip, and the trajectory-observer safe_preview borrow fix. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * style(reborn): wrap an over-long line for rustfmt 1.9.0 CI installs the latest stable rustfmt (1.9.0 / Rust 1.96), which wraps a long eprintln! that older rustfmt left inline. Fixes the Formatting check. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * style(reborn): collapse nested if for clippy 1.96 collapsible_if clippy 1.96 (CI's stable) flags the nested if-let in the final-answer-nudge site as collapsible; fold it into a let-chain. No behaviour change. Fixes Clippy (all-features). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * WIP(bench): multi-tool-call port reuse (matches main nearai#4790) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WIP(bench): nudge isolation - disable gate to measure marginal contribution * Revert stray bench WIP accidentally committed onto this branch Removes the http/nudge/multi-tool-call/diagnostic WIP commits (c4bbb5f, 2c670b4, 6da818a) that were committed onto the reborn-trajectory-observer branch by mistake during benchmarking and swept to origin by a main-merge push. Restores the affected files to origin/main (multi-tool-call is already fixed there by nearai#4790; the http fix lives in PR nearai#4827). Observer-owned changes in state.rs, refreshing_capability_port.rs, and local_dev.rs are preserved minus the stray WIP additions. No history rewrite / force-push. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(reborn): preserve provider factory across reload + reject observer off local-dev Addresses Firat's review of the trajectory/provider seams (nearai#4588): - Provider factory now survives a live config reload. build_llm_gateway applied the factory to the bare config provider *before* wrapping it in the SwappableLlmProvider, so the first WebUI/settings reload (which swaps the swappable's inner) silently dropped the instrumentation wrapper. Invert the layering: build the config provider, put it behind the swappable + reload handle, then apply the factory *over the swappable* for the gateway-facing provider. Reloads swap the inner; the wrapper stays in the call path. Regression test provider_factory_survives_live_reload reloads and proves the wrapper still observes subsequent model calls. - Reject a trajectory observer on profiles without a local runtime. The observer is wired only through the local-dev capability path; Production silently dropped it, so a caller got an empty trajectory with no error. Fail fast with InvalidArgument and document the seam as local-dev/bench-only. Test build_reborn_runtime_rejects_trajectory_observer_for_production. cargo fmt + clippy (all-features, -D warnings) clean under rustfmt 1.9/clippy 1.96. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs(reborn): note trajectory observer is local-dev/bench-only Document the local-dev-only constraint + fail-fast behavior on the public with_trajectory_observer / with_raw_trajectory_observer setters (Firat review). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> Co-authored-by: Pranav Raja <pranav.raja@near.ai>
chenzhongsong93
pushed a commit
to chenzhongsong93/ironclaw
that referenced
this pull request
Sep 23, 2026
chenzhongsong93
pushed a commit
to chenzhongsong93/ironclaw
that referenced
this pull request
Sep 24, 2026
chenzhongsong93
pushed a commit
to chenzhongsong93/ironclaw
that referenced
this pull request
Sep 26, 2026
chenzhongsong93
pushed a commit
to chenzhongsong93/ironclaw
that referenced
this pull request
Sep 26, 2026
chenzhongsong93
pushed a commit
to chenzhongsong93/ironclaw
that referenced
this pull request
Sep 26, 2026
chenzhongsong93
pushed a commit
to chenzhongsong93/ironclaw
that referenced
this pull request
Sep 26, 2026
chenzhongsong93
pushed a commit
to chenzhongsong93/ironclaw
that referenced
this pull request
Sep 26, 2026
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.
Checking out main branch; noticed:
near:agent/channeldoes not have exporton-status"After all these, things run smooth!