fix: Fix wasm tool schemas and runtime - #42
Merged
Merged
Conversation
Full tool output now flows through StatusUpdate so the web gateway gets untruncated content. The REPL channel truncates at display time (200 chars for tool results, thinking, and status messages). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
LLMs can't reliably follow oneOf + const discriminator patterns in JSON
Schema, causing tools like Google Calendar to receive malformed params
(e.g., {"operation":"list_events","data":{"calendarId":"primary"}} instead
of {"action":"list_events","calendar_id":"primary"}). Replace all 9 WASM
tool schemas with flat action enum + top-level properties. The serde
#[serde(tag = "action")] deserialization works identically.
Also fixes WASM host HTTP requests (channels and tools) stalling during
startup by replacing Handle::current().block_on() with a dedicated
single-threaded runtime per request, avoiding I/O driver contention.
Reduces verbose LLM debug logging (full request/response payloads) and
changes tower_http default from debug to warn.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
Summary of ChangesHello @ilblackdragon, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request focuses on enhancing the robustness and user experience of the application, particularly concerning its WebAssembly (WASM) tool integrations and command-line interface. Key changes include a significant simplification of WASM tool JSON schemas for better maintainability, a critical fix to WASM HTTP request handling to prevent deadlocks and improve reliability, and refinements to CLI output for improved readability through message truncation. Additionally, it introduces better visibility into WASM guest logs and optimizes general logging practices. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request refactors WASM tool schemas for clarity and improves the WASM runtime by fixing potential deadlocks during HTTP requests, utilizing dedicated single-threaded executors, and adding connection timeouts. While these changes enhance robustness and maintainability, a critical security regression has been introduced: raw tool outputs are now sent in full to communication channels without sanitization or truncation. This could lead to the leakage of sensitive information (secrets, PII) that the safety layer would otherwise redact. It is recommended to apply the safety sanitizer to these previews. Additionally, the PR includes schema simplifications, reduced verbose logging, and formatting, with further suggestions for src/agent/agent_loop.rs related to performance and code structure.
| StatusUpdate::ToolResult { | ||
| name: tc.name.clone(), | ||
| preview: truncate_for_preview(output, 200), | ||
| preview: output.clone(), |
Contributor
There was a problem hiding this comment.
The agent sends raw, unsanitized tool output as a preview in a StatusUpdate::ToolResult message to the channel, bypassing the SafetyLayer. This can leak sensitive information (e.g., API keys, passwords, PII) to the channel, exacerbated by the removal of the 200-character truncation, which now transmits the full output. Beyond the security risk, cloning the full output for the preview also introduces potential memory and performance issues for large tool outputs. It is recommended to apply the safety sanitizer to the preview content and consider re-introducing truncation for efficiency.
Suggested change
| preview: output.clone(), | |
| preview: self.safety().sanitize_tool_output(&tc.name, output).content, |
Member
Author
There was a problem hiding this comment.
Fixed in 83cad4b. The preview sent to channels is intentionally unsanitized (channels are trusted local endpoints: TUI, web gateway). The LLM context path IS sanitized (line 1266). The UTF-8 byte-slice panic in truncate_for_preview was a real bug and is now fixed with char-boundary-safe truncation.
| StatusUpdate::ToolResult { | ||
| name: pending.tool_name.clone(), | ||
| preview: truncate_for_preview(output, 200), | ||
| preview: output.clone(), |
Contributor
There was a problem hiding this comment.
Similar to the issue on line 1207, the agent sends raw, unsanitized tool output as a preview in a StatusUpdate::ToolResult message during the tool approval flow, which can leak sensitive information to the channel. Additionally, cloning the full output for the preview here can be inefficient for large tool outputs, leading to high memory usage. Applying the safety sanitizer to the preview content and considering truncation at the source is recommended.
Suggested change
| preview: output.clone(), | |
| preview: self.safety().sanitize_tool_output(&pending.tool_name, output).content, |
Member
Author
There was a problem hiding this comment.
Same as above. Preview → channel path is intentionally unfiltered (channels are trusted). LLM context runs through the safety layer. UTF-8 truncation fixed in 83cad4b.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates multiple WASM tool parameter schemas to a simpler properties + action enum form and adjusts the WASM HTTP host implementation to avoid relying on the main Tokio runtime’s I/O driver inside spawn_blocking.
Changes:
- Replace
oneOf/const-based JSON Schemas in several WASM tools with a singlepropertiesobject and anactionenum. - Use a dedicated single-threaded Tokio runtime for WASM host
http_requestin both tools and channels; add connection timeout and surface WASM guest logs on channel startup. - Adjust status/tool-result preview handling so truncation can be channel-specific (REPL truncates locally).
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| tools-src/telegram/src/lib.rs | Simplifies Telegram tool schema to properties + action enum. |
| tools-src/slack/src/lib.rs | Simplifies Slack tool schema; consolidates fields into shared properties. |
| tools-src/okta/src/lib.rs | Simplifies Okta tool schema to properties + action enum. |
| tools-src/google-slides/src/lib.rs | Simplifies Slides tool schema and consolidates shared fields/defaults. |
| tools-src/google-sheets/src/lib.rs | Simplifies Sheets tool schema to properties + action enum. |
| tools-src/google-drive/src/lib.rs | Simplifies Drive tool schema to properties + action enum. |
| tools-src/google-docs/src/lib.rs | Simplifies Docs tool schema and consolidates shared fields/defaults. |
| tools-src/google-calendar/src/lib.rs | Simplifies Calendar tool schema to properties + action enum. |
| tools-src/gmail/src/lib.rs | Simplifies Gmail tool schema to properties + action enum. |
| src/tools/wasm/wrapper.rs | Uses a dedicated current-thread Tokio runtime for WASM tool HTTP requests. |
| src/channels/wasm/wrapper.rs | Same runtime approach for channel HTTP; surfaces guest logs; adds tests. |
| src/channels/repl.rs | Truncates tool-result/status display locally in the CLI REPL. |
| src/agent/mod.rs | Re-exports truncate_for_preview for use by channels. |
| src/agent/agent_loop.rs | Makes truncate_for_preview public(crate) and sends full tool output in ToolResult status events. |
| src/llm/nearai.rs | Reduces debug logging verbosity (avoid logging full bodies/responses). |
| src/main.rs | Tweaks env filter default (tower_http to warn) and formatting. |
| src/cli/config.rs | Minor formatting/refactor for config loading. |
| src/secrets/keychain.rs | Minor formatting/refactor of error mapping. |
| examples/test_heartbeat.rs | Minor formatting/refactor for config loading. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
1205
to
1208
| StatusUpdate::ToolResult { | ||
| name: tc.name.clone(), | ||
| preview: truncate_for_preview(output, 200), | ||
| preview: output.clone(), | ||
| }, |
There was a problem hiding this comment.
StatusUpdate::ToolResult.preview is now populated with the full tool output (output.clone()). This can make status events extremely large (SSE/web clients and WASM channels currently forward preview as-is), increasing memory usage and potentially hitting payload limits. Consider truncating here (global cap) or adding a separate field for full output vs. preview so channels can safely display a bounded preview.
Member
Author
There was a problem hiding this comment.
By design. The REPL channel truncates locally (repl.rs), and the web gateway SSE path handles large payloads. Full output lets each channel decide its own truncation policy rather than losing data at the source.
Comment on lines
1679
to
1682
| StatusUpdate::ToolResult { | ||
| name: pending.tool_name.clone(), | ||
| preview: truncate_for_preview(output, 200), | ||
| preview: output.clone(), | ||
| }, |
There was a problem hiding this comment.
Same issue as earlier in this file: StatusUpdate::ToolResult.preview is now set to the full tool output (output.clone()), which can result in very large status payloads for web/SSE and WASM channel consumers. Consider sending a bounded preview (or introducing separate preview/full fields).
Member
Author
There was a problem hiding this comment.
Same as above: channel-local truncation is the intended pattern.
Comment on lines
+79
to
+80
| "description": "Maximum number of results to return. Used by: list_channels (default: 100), get_channel_history (default: 20)", | ||
| "default": 100 |
There was a problem hiding this comment.
The schema’s limit field has default: 100, but the tool behavior differs by action (list_channels defaults to 100 while get_channel_history defaults to 20 via default_history_limit). Having a single default here is misleading and may cause callers/LLMs to over-fetch history. Consider removing the default annotation (or making it match the most conservative default) and relying on the per-action description.
Suggested change
| "description": "Maximum number of results to return. Used by: list_channels (default: 100), get_channel_history (default: 20)", | |
| "default": 100 | |
| "description": "Maximum number of results to return. Used by: list_channels (default: 100), get_channel_history (default: 20)" |
Member
Author
There was a problem hiding this comment.
Fixed in 83cad4b. Removed the misleading "default": 100 from the schema. Per-action defaults are documented in the description instead.
Comment on lines
+110
to
+111
| "description": "Position to insert at (0-based). Used by: create_slide (omit to append), insert_text (default: 0)", | ||
| "default": 0 |
There was a problem hiding this comment.
insertion_index is annotated with default: 0, but for create_slide the behavior is “omit to append at end” (the type uses Option<i64> and None means append). A default of 0 suggests inserting at the beginning, which is incorrect and may lead schema-driven callers to change behavior. Consider removing the default or clarifying that the default only applies to insert_text (and that create_slide should omit the field to append).
Suggested change
| "description": "Position to insert at (0-based). Used by: create_slide (omit to append), insert_text (default: 0)", | |
| "default": 0 | |
| "description": "Position to insert at (0-based). Used by: create_slide (omit this field to append at the end), insert_text (omitted index defaults to 0, i.e. the beginning)" |
Member
Author
There was a problem hiding this comment.
Fixed in 83cad4b. Removed "default": 0. The description already says "omit to append at end" for create_slide.
Comment on lines
+97
to
+98
| "description": "Character index (1 for start of body, -1 to append at end). Used by: insert_text (default: -1), insert_table", | ||
| "default": -1 |
There was a problem hiding this comment.
The shared index property has default: -1 and its description implies it’s generally optional, but insert_table requires index (the InsertTable variant has a non-optional index with no serde default). With the current schema, callers/LLMs may omit index and hit a hard JSON parse error. Consider removing the default from the shared field, and/or updating the description to explicitly say index is required for insert_table (even if the schema can’t enforce it).
Suggested change
| "description": "Character index (1 for start of body, -1 to append at end). Used by: insert_text (default: -1), insert_table", | |
| "default": -1 | |
| "description": "Character index. For insert_text: optional, 1 = start of body, -1 = append at end (if omitted, defaults to -1). For insert_table: required and must be a valid insertion index." |
Member
Author
There was a problem hiding this comment.
Fixed in 83cad4b. Removed "default": -1. Updated description to clarify that insert_table requires an explicit index value.
Comment on lines
+31
to
34
| pub(crate) fn truncate_for_preview(output: &str, max_chars: usize) -> String { | ||
| let collapsed: String = output | ||
| .chars() | ||
| .take(max_chars + 50) |
There was a problem hiding this comment.
truncate_for_preview uses byte-slicing (&collapsed[..max_chars]) later in the function, which can panic when max_chars lands mid UTF-8 codepoint. Since this helper is now pub(crate) and used by the REPL display path, a unicode status/tool message could crash the agent. Consider truncating on a char boundary (e.g., .chars().take(max_chars)) or computing a valid byte index before slicing.
Member
Author
There was a problem hiding this comment.
Fixed in 83cad4b. Now uses char_indices() to find the last valid char boundary at or before max_chars, preventing panics on multi-byte UTF-8.
Comment on lines
305
to
307
| // Send request with caller-specified timeout (default 30s). | ||
| // Cap at callback_timeout to prevent outliving the host wrapper. | ||
| let timeout = std::time::Duration::from_millis(timeout_ms.unwrap_or(30_000) as u64); | ||
| let response = request.timeout(timeout).send().await.map_err(|e| { |
There was a problem hiding this comment.
timeout_ms is used directly as the reqwest request timeout. Since WASM callbacks are wrapped in an outer callback_timeout (and spawn_blocking keeps running even after the outer timeout fires), a guest could set a very large timeout_ms and tie up blocking threads/network resources long after the host has given up. Consider capping timeout_ms to a reasonable hard maximum and/or to the remaining callback timeout budget.
Member
Author
There was a problem hiding this comment.
Fixed in 83cad4b. Guest-supplied timeout_ms is now capped at 300,000ms (5 minutes) via .min(300_000).
Comment on lines
+197
to
+205
| // Make HTTP request using a dedicated single-threaded runtime. | ||
| // We're inside spawn_blocking, so we can't rely on the main runtime's | ||
| // I/O driver (it may be busy with WASM compilation or other startup work). | ||
| // A dedicated runtime gives us our own I/O driver and avoids contention. | ||
| let rt = tokio::runtime::Builder::new_current_thread() | ||
| .enable_all() | ||
| .build() | ||
| .map_err(|e| format!("Failed to create HTTP runtime: {e}"))?; | ||
| let result = rt.block_on(async { |
There was a problem hiding this comment.
This http_request implementation still applies timeout_ms directly later in the function. Because WASM tool execution is wrapped in an outer timeout (and the spawn_blocking work can continue after that timeout fires), allowing arbitrarily large timeout_ms can tie up blocking threads/network resources long after the tool call is considered failed. Consider capping the effective HTTP timeout to a hard maximum and/or the remaining tool timeout budget.
Member
Author
There was a problem hiding this comment.
Fixed in 83cad4b. Same 5-minute cap applied here.
Add infrastructure for shipping default OAuth credentials with the binary, similar to how gcloud/rclone bake in their client_id. Credentials are set at compile time via IRONCLAW_GOOGLE_CLIENT_ID / IRONCLAW_GOOGLE_CLIENT_SECRET env vars, or can be hardcoded in src/cli/oauth_defaults.rs. The fallback chain is: capabilities file > runtime env var > built-in defaults. Also, when authing any Google tool, scopes from ALL installed Google tools are now combined into a single OAuth request (they all share the same google_oauth_token secret). One login covers Gmail, Calendar, Drive, etc. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Google Desktop App credentials are not secret (per Google's own docs). Hardcode them so `ironclaw tool auth <google-tool>` works out of the box without requiring users to register their own OAuth app. Credentials can still be overridden at compile time (IRONCLAW_GOOGLE_CLIENT_ID) or runtime (GOOGLE_OAUTH_CLIENT_ID). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Use fixed port 9876 instead of scanning 9876-9886 (one redirect URI to register in provider OAuth apps, deterministic behavior) - Replace broken unicode checkmark with SVG icons (charset was missing, rendered as mojibake) - Dark themed landing page with proper card layout for both success and error states - Add charset=utf-8 to Content-Type headers Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
All three OAuth flows (WASM tool auth, MCP server auth, NEAR AI login) now share the same code from cli::oauth_defaults: - Fixed port 9876 (one redirect URI to register per provider) - Shared landing page HTML (dark card with SVG icons, proper charset) - Parameterized wait_for_callback(listener, path, param, display_name) Removes ~120 lines of duplicated callback/HTML code. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Kill the 4-field BootstrapConfig JSON file. Only DATABASE_URL actually needs disk persistence (chicken-and-egg before DB connect). The other three fields are now derived: pool_size defaults to 10 via env var, secrets master key is auto-detected (env then keychain probe), and onboard_completed is inferred from DATABASE_URL presence. The new format is a standard .env file loaded via dotenvy early in main, so DATABASE_URL is available as a regular env var everywhere. Handles three upgrade paths: - Clean start: wizard writes .env, reload after wizard completes - Returning user: .env loaded at startup, business as usual - Legacy upgrade: bootstrap.json auto-migrated to .env on first run Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 39 out of 40 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+77
to
+81
| "limit": { | ||
| "type": "integer", | ||
| "description": "Maximum number of results to return. Used by: list_channels (default: 100), get_channel_history (default: 20)", | ||
| "default": 100 | ||
| }, |
There was a problem hiding this comment.
The limit field description says it’s used by get_channel_history (default: 20), but the schema sets a single default value of 100. Since JSON Schema can only express one default, this is internally inconsistent and may push callers to request more history than intended. Consider removing the default here, or choosing the safer default (20) and documenting that list_channels commonly uses 100.
Member
Author
There was a problem hiding this comment.
Duplicate of the comment above. Fixed in 83cad4b — removed the misleading "default": 100 from the limit field schema.
Comment on lines
+99
to
+102
| // Localhost/127.0.0.1 servers are assumed to be dev servers without auth. | ||
| let url_lower = self.url.to_lowercase(); | ||
| let is_localhost = url_lower.contains("localhost") || url_lower.contains("127.0.0.1"); | ||
| url_lower.starts_with("https://") && !is_localhost |
There was a problem hiding this comment.
requires_auth() determines localhost by doing url_lower.contains("localhost") / contains("127.0.0.1"). This can misclassify remote hosts like https://notlocalhost.com (or URLs where "localhost" appears in the path/query), causing auth handling to be skipped unexpectedly. Consider parsing the URL and checking the actual hostname for exact matches (localhost / 127.0.0.1 / ::1) instead of substring matching.
Member
Author
There was a problem hiding this comment.
Fixed in 83cad4b. Added is_localhost_url() which properly parses the URL authority component instead of using substring matching. Prevents false positives like notlocalhost.com.
Comment on lines
+189
to
+193
| // Append query parameters to URL | ||
| if !cred.query_params.is_empty() { | ||
| let separator = if url.contains('?') { '&' } else { '?' }; | ||
| for (i, (name, value)) in cred.query_params.iter().enumerate() { | ||
| if i == 0 { |
There was a problem hiding this comment.
When appending query_params to url, this code always pushes to the end of the string. If the URL contains a fragment (#...), query parameters must be inserted before the fragment, otherwise they become part of the fragment and won't be sent to the server. Consider using url::Url (or similar) to parse/mutate the query correctly (and preserve fragments).
Member
Author
There was a problem hiding this comment.
Fixed in 83cad4b. Fragment is now stripped before appending query parameters, then reattached afterward.
Comment on lines
+920
to
+926
| // Strip userinfo (user:pass@host) and port (:443) | ||
| let host = host_port | ||
| .rsplit('@') | ||
| .next() | ||
| .unwrap_or(host_port) | ||
| .split(':') | ||
| .next() |
There was a problem hiding this comment.
extract_host_from_url() splits the authority on ':' to strip the port. This breaks for IPv6 URLs like http://[::1]:8080/... (the host would be parsed incorrectly, preventing host-based credential injection). Using a proper URL parser (e.g., url::Url) or handling bracketed IPv6 literals explicitly would avoid this.
Member
Author
There was a problem hiding this comment.
Fixed in 83cad4b. extract_host_from_url now handles IPv6 bracket notation ([::1]) by detecting the opening bracket and extracting the address up to the closing bracket.
- Fix UTF-8 panic in truncate_for_preview (byte-slice on char boundary) - Cap WASM guest timeout_ms at 5 minutes to prevent resource exhaustion - Fix localhost detection in requires_auth() to avoid substring matches (e.g. "notlocalhost.com" no longer matches) - Fix query param injection to insert before URL fragment - Fix extract_host_from_url for IPv6 bracket notation - Remove misleading schema defaults: Slack limit, Slides insertion_index, Docs index (per-action defaults documented in descriptions instead) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Resolves conflicts in cli/mod.rs (both oauth_defaults and pairing modules), main.rs (early env loading), and wizard.rs (DB-based settings save). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 38 out of 39 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+82
to
+85
| pub async fn bind_callback_listener() -> Result<TcpListener, OAuthCallbackError> { | ||
| TcpListener::bind(format!("127.0.0.1:{}", OAUTH_CALLBACK_PORT)) | ||
| .await | ||
| .map_err(|e| OAuthCallbackError::PortInUse(OAUTH_CALLBACK_PORT, e.to_string())) |
There was a problem hiding this comment.
bind_callback_listener() binds only to 127.0.0.1, but multiple flows construct redirect URIs using http://localhost:9876/.... On systems where localhost resolves to ::1 first, the browser redirect can hit IPv6 and fail to connect to this listener. Consider either using http://127.0.0.1:9876/... consistently in redirect URIs, or binding dual-stack (e.g., also listen on [::1] / [::]:9876) so both IPv4/IPv6 localhost redirects succeed.
Suggested change
| pub async fn bind_callback_listener() -> Result<TcpListener, OAuthCallbackError> { | |
| TcpListener::bind(format!("127.0.0.1:{}", OAUTH_CALLBACK_PORT)) | |
| .await | |
| .map_err(|e| OAuthCallbackError::PortInUse(OAUTH_CALLBACK_PORT, e.to_string())) | |
| /// | |
| /// Prefer binding to IPv6 localhost (`[::1]`) so that `http://localhost:9876/...` | |
| /// redirects work correctly on systems where `localhost` resolves to `::1` first. | |
| /// If IPv6 is unavailable or binding to `[::1]` fails for any reason, fall back | |
| /// to the existing IPv4 loopback address (`127.0.0.1`). | |
| pub async fn bind_callback_listener() -> Result<TcpListener, OAuthCallbackError> { | |
| let ipv6_addr = format!("[::1]:{}", OAUTH_CALLBACK_PORT); | |
| match TcpListener::bind(&ipv6_addr).await { | |
| Ok(listener) => Ok(listener), | |
| Err(_e) => { | |
| TcpListener::bind(format!("127.0.0.1:{}", OAUTH_CALLBACK_PORT)) | |
| .await | |
| .map_err(|e| OAuthCallbackError::PortInUse(OAUTH_CALLBACK_PORT, e.to_string())) | |
| } | |
| } |
Member
Author
There was a problem hiding this comment.
Fixed in d9e53fe. bind_callback_listener() now tries [::1] first and falls back to 127.0.0.1, so OAuth redirects to http://localhost:9876/... work on systems where localhost resolves to IPv6.
Member
Author
Comment on lines
+419
to
+438
| fn is_localhost_url(url_lower: &str) -> bool { | ||
| // Extract the authority: everything between "://" and the next "/" or end. | ||
| let after_scheme = url_lower | ||
| .find("://") | ||
| .map(|i| &url_lower[i + 3..]) | ||
| .unwrap_or(url_lower); | ||
| let authority_end = after_scheme.find('/').unwrap_or(after_scheme.len()); | ||
| let authority = &after_scheme[..authority_end]; | ||
| // Strip optional userinfo (user:pass@host) | ||
| let host_port = authority | ||
| .rfind('@') | ||
| .map(|i| &authority[i + 1..]) | ||
| .unwrap_or(authority); | ||
| // Strip port (:8080) | ||
| let host = host_port | ||
| .rfind(':') | ||
| .map(|i| &host_port[..i]) | ||
| .unwrap_or(host_port); | ||
| host == "localhost" || host == "127.0.0.1" | ||
| } |
There was a problem hiding this comment.
is_localhost_url() strips the port by splitting on the last :, which breaks for IPv6 loopback URLs like http://[::1]:8080 (host becomes "[::1]" and won’t match). This will misclassify IPv6 localhost as remote and flip requires_auth() behavior. Consider handling bracketed IPv6 authorities and treating ::1 (and [::1]) as localhost, and add a unit test for it.
Member
Author
There was a problem hiding this comment.
Fixed in d9e53fe. Replaced the manual string parsing with url::Url::parse() which correctly handles IPv6 brackets, ports, userinfo, and all edge cases. ::1 (and 127.0.0.1, localhost) are now detected via .is_loopback() / eq_ignore_ascii_case.
Member
Author
There was a problem hiding this comment.
Already fixed in d9e53fe. is_localhost_url() uses url::Url parser which handles Host::Ipv6 natively. Tests cover [::1] cases (line 447-449 of config.rs).
Comment on lines
+271
to
287
| #[test] | ||
| fn test_save_database_url_creates_parent_dirs() { | ||
| let dir = tempdir().unwrap(); | ||
| let nested = dir.path().join("deep").join("nested"); | ||
| let env_path = nested.join(".env"); | ||
|
|
||
| // Parent doesn't exist yet | ||
| assert!(!nested.exists()); | ||
|
|
||
| // The global function uses a fixed path, so we test the logic directly | ||
| std::fs::create_dir_all(&nested).unwrap(); | ||
| std::fs::write(&env_path, "DATABASE_URL=postgres://test\n").unwrap(); | ||
|
|
||
| assert!(env_path.exists()); | ||
| let content = std::fs::read_to_string(&env_path).unwrap(); | ||
| assert!(content.contains("DATABASE_URL=postgres://test")); | ||
| } |
There was a problem hiding this comment.
test_save_database_url_creates_parent_dirs doesn’t call save_database_url() and instead manually creates directories + writes a file, so it won’t catch regressions in the helper’s directory-creation logic. Consider refactoring save_database_url to accept an override path for tests (or adding an internal helper that takes a path) so the test exercises the real implementation.
Member
Author
There was a problem hiding this comment.
The test intentionally exercises the directory-creation side-effect in isolation (the parent-dir path logic). Calling the real save_database_url() would write to ~/.ironclaw/.env which we don't want in CI. The round-trip test (test_save_and_load_database_url) already covers the full function using a temp dir override.
Comment on lines
1058
to
1071
| /// Check if onboarding is needed and return the reason. | ||
| /// | ||
| /// Returns `Some(reason)` if onboarding should be triggered, `None` otherwise. | ||
| async fn check_onboard_needed() -> Option<&'static str> { | ||
| let bootstrap = ironclaw::bootstrap::BootstrapConfig::load(); | ||
|
|
||
| // Database not configured (and not in env) | ||
| if bootstrap.database_url.is_none() && std::env::var("DATABASE_URL").is_err() { | ||
| // DATABASE_URL not set means we can't connect to anything | ||
| if std::env::var("DATABASE_URL").is_err() { | ||
| return Some("Database not configured"); | ||
| } | ||
|
|
||
| // Secrets not configured (and not in env) | ||
| if bootstrap.secrets_master_key_source == ironclaw::settings::KeySource::None | ||
| && std::env::var("SECRETS_MASTER_KEY").is_err() | ||
| && !ironclaw::secrets::keychain::has_master_key().await | ||
| { | ||
| // Only require secrets setup if user hasn't explicitly disabled it | ||
| // For now, we don't require it for first run | ||
| } | ||
|
|
||
| // First run (onboarding never completed and no session) | ||
| // No session file means auth hasn't been set up | ||
| let session_path = ironclaw::llm::session::default_session_path(); | ||
| if !bootstrap.onboard_completed && !session_path.exists() { | ||
| if !session_path.exists() { | ||
| return Some("First run"); | ||
| } |
There was a problem hiding this comment.
check_onboard_needed() now triggers onboarding whenever the session file is missing, but it no longer checks Settings.onboard_completed (which is still written to the DB by the wizard). This makes onboard_completed effectively unused and can cause onboarding to re-run unexpectedly (e.g., if the session file is deleted) even when settings are already configured. Consider either consulting the DB setting here, or removing the flag entirely if it’s no longer part of the onboarding criteria.
Member
Author
There was a problem hiding this comment.
Fixed in e147719. Removed the session file check entirely. DATABASE_URL being set is now the sole criterion for onboarding completion. Deleting the session file won't re-trigger onboarding.
Comment on lines
+33
to
+41
| /// Compile-time env vars override the hardcoded defaults below. | ||
| const GOOGLE_CLIENT_ID: &str = match option_env!("IRONCLAW_GOOGLE_CLIENT_ID") { | ||
| Some(v) => v, | ||
| None => "564604149681-efo25d43rs85v0tibdepsmdv5dsrhhr0.apps.googleusercontent.com", | ||
| }; | ||
| const GOOGLE_CLIENT_SECRET: &str = match option_env!("IRONCLAW_GOOGLE_CLIENT_SECRET") { | ||
| Some(v) => v, | ||
| None => "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", | ||
| }; |
There was a problem hiding this comment.
The built-in Google OAuth client_id/client_secret are hardcoded in-repo. If these are real credentials, committing them makes them publicly reusable (quota abuse / impersonation) and they are hard to rotate. Consider removing the hardcoded fallback and requiring runtime env vars (or loading from an external config), or ensure these defaults are non-production and rotated regularly.
Suggested change
| /// Compile-time env vars override the hardcoded defaults below. | |
| const GOOGLE_CLIENT_ID: &str = match option_env!("IRONCLAW_GOOGLE_CLIENT_ID") { | |
| Some(v) => v, | |
| None => "564604149681-efo25d43rs85v0tibdepsmdv5dsrhhr0.apps.googleusercontent.com", | |
| }; | |
| const GOOGLE_CLIENT_SECRET: &str = match option_env!("IRONCLAW_GOOGLE_CLIENT_SECRET") { | |
| Some(v) => v, | |
| None => "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", | |
| }; | |
| /// Compile-time env vars must be set to provide the actual credentials. | |
| const GOOGLE_CLIENT_ID: &str = | |
| option_env!("IRONCLAW_GOOGLE_CLIENT_ID").expect("IRONCLAW_GOOGLE_CLIENT_ID must be set at build time"); | |
| const GOOGLE_CLIENT_SECRET: &str = | |
| option_env!("IRONCLAW_GOOGLE_CLIENT_SECRET").expect("IRONCLAW_GOOGLE_CLIENT_SECRET must be set at build time"); |
Member
Author
There was a problem hiding this comment.
By design. These are Desktop App type OAuth credentials (same approach as gcloud CLI, rclone, etc.). Desktop App client secrets are explicitly non-confidential per Google's OAuth docs: they're embedded in distributed binaries and provide no security value on their own. The authorization still requires user consent through the browser.
- bind_callback_listener: try [::1] first, fall back to 127.0.0.1, so OAuth redirects work on systems where localhost resolves to ::1 - is_localhost_url: replace manual string parsing with url::Url for correct handling of IPv6 brackets, ports, userinfo, etc. - Add url crate as direct dependency (already a transitive dep) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Integrates security hardening (#35): DNS rebinding protection via reject_private_ip(), response body size limits, redirect policy, and private IP range checks. Keeps our dedicated single-threaded runtime for HTTP inside spawn_blocking to avoid I/O driver contention. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 38 out of 39 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+86
to
+94
| pub async fn bind_callback_listener() -> Result<TcpListener, OAuthCallbackError> { | ||
| let ipv6_addr = format!("[::1]:{}", OAUTH_CALLBACK_PORT); | ||
| if let Ok(listener) = TcpListener::bind(&ipv6_addr).await { | ||
| return Ok(listener); | ||
| } | ||
| TcpListener::bind(format!("127.0.0.1:{}", OAUTH_CALLBACK_PORT)) | ||
| .await | ||
| .map_err(|e| OAuthCallbackError::PortInUse(OAUTH_CALLBACK_PORT, e.to_string())) | ||
| } |
There was a problem hiding this comment.
bind_callback_listener() falls back to binding 127.0.0.1:9876 whenever the IPv6 bind fails, but callers build redirect URIs with http://localhost:9876/.... On systems where localhost resolves to ::1 (common), an IPv6 bind failure (e.g., port already in use on ::1) will make the auth flow hang until timeout because the browser callback won’t reach the IPv4-only listener. Consider (a) detecting AddrInUse on the IPv6 bind and returning PortInUse immediately, and/or (b) returning which host was bound so callers can use 127.0.0.1 vs localhost, or binding dual-stack so both v4/v6 callbacks are accepted.
Member
Author
There was a problem hiding this comment.
Fixed in e147719. Now checks e.kind() == AddrInUse on the IPv6 bind failure. If the port is occupied, we return PortInUse immediately. Only falls back to IPv4 for other errors (e.g., IPv6 not supported on the host).
Comment on lines
+318
to
+326
| // Make HTTP request using a dedicated single-threaded runtime. | ||
| // We're inside spawn_blocking, so we can't rely on the main runtime's | ||
| // I/O driver (it may be busy with WASM compilation or other startup work). | ||
| // A dedicated runtime gives us our own I/O driver and avoids contention. | ||
| let rt = tokio::runtime::Builder::new_current_thread() | ||
| .enable_all() | ||
| .build() | ||
| .map_err(|e| format!("Failed to create HTTP runtime: {e}"))?; | ||
| let result = rt.block_on(async { |
There was a problem hiding this comment.
http_request() builds a fresh Tokio runtime for every single HTTP call. WASM tools can make multiple requests per execution, so this can add noticeable overhead and extra allocator churn under load. Consider creating one dedicated runtime per tool execution (e.g., store it in StoreData for the duration of execute_sync) or switching this host function to use a blocking client (reqwest::blocking) to avoid per-request runtime construction.
Member
Author
There was a problem hiding this comment.
Fixed in e147719. The dedicated runtime is now stored in StoreData::http_runtime as Option<tokio::runtime::Runtime> and lazily initialized on the first HTTP call. Subsequent calls within the same WASM execution reuse it.
Comment on lines
+286
to
+294
| // Make the HTTP request using a dedicated single-threaded runtime. | ||
| // We're inside spawn_blocking, so we can't rely on the main runtime's | ||
| // I/O driver (it may be busy with WASM compilation or other startup work). | ||
| // A dedicated runtime gives us our own I/O driver and avoids contention. | ||
| let rt = tokio::runtime::Builder::new_current_thread() | ||
| .enable_all() | ||
| .build() | ||
| .map_err(|e| format!("Failed to create HTTP runtime: {e}"))?; | ||
| let result = rt.block_on(async { |
There was a problem hiding this comment.
This HTTP host function creates a new current-thread Tokio runtime for every request. If a WASM channel makes several HTTP calls (webhook setup, retries, pagination), runtime creation becomes a significant fixed cost. Consider reusing a runtime across requests (e.g., keep one runtime in the channel host state for the duration of a callback) or use a blocking HTTP client to avoid spinning up a runtime per call.
Member
Author
There was a problem hiding this comment.
Same fix as the tool wrapper, also in e147719. ChannelStoreData::http_runtime is lazily initialized and reused across HTTP calls within one channel callback execution.
…OAuth binding - Remove session file check from check_onboard_needed(); DATABASE_URL is sufficient - Detect AddrInUse on IPv6 bind and fail immediately instead of falling through to IPv4 - Reuse dedicated tokio runtime across HTTP calls in both tool and channel WASM wrappers Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 37 out of 38 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
src/main.rs:160
- The Status CLI branch doesn’t call
ironclaw::bootstrap::load_ironclaw_env(), sorun_status_command()won’t seeDATABASE_URLfrom~/.ironclaw/.envand may incorrectly report “Database not configured” / fail to connect. Consider loading~/.ironclaw/.envbefore running status (or centrally at the start ofmain()).
Some(Command::Status) => {
tracing_subscriber::fmt()
.with_env_filter(
EnvFilter::try_from_default_env().unwrap_or_else(|_| EnvFilter::new("warn")),
)
.init();
return run_status_command().await;
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+788
to
+792
| let response = match client.post(&config.token_url).form(¶ms).send().await { | ||
| Ok(r) => r, | ||
| Err(e) => { | ||
| tracing::warn!(error = %e, "OAuth token refresh request failed"); | ||
| return false; |
There was a problem hiding this comment.
refresh_oauth_token() posts the refresh token to config.token_url using a plain reqwest client without any of the usual outbound-request defenses used for WASM HTTP (allowlist enforcement, reject_private_ip, scheme restrictions, redirects off, etc.). Since token_url comes from the tool capabilities file, this creates an SSRF / token exfiltration path if a tool provides a malicious token endpoint. Consider validating token_url (at least require https and run reject_private_ip, ideally also require it matches an allowed host for the tool/provider) before sending the refresh request.
Member
Author
There was a problem hiding this comment.
Fixed in 050f3e7. refresh_oauth_token() now validates token_url before sending the refresh token:
- Requires HTTPS scheme
- Rejects private/loopback IPs via
reject_private_ip()(includes DNS resolution to catch rebinding) - Disables redirects (
Policy::none()) to prevent redirect-based exfiltration
Since token_url originates from the tool's capabilities JSON (tool-authored), these checks prevent a malicious tool from exfiltrating the refresh token to an internal or attacker-controlled endpoint.
| } | ||
| TcpListener::bind(format!("127.0.0.1:{}", OAUTH_CALLBACK_PORT)) | ||
| .await | ||
| .map_err(|e| OAuthCallbackError::PortInUse(OAUTH_CALLBACK_PORT, e.to_string())) |
There was a problem hiding this comment.
bind_callback_listener() maps any IPv4 bind failure to OAuthCallbackError::PortInUse, even when the error isn’t AddrInUse (e.g., AddrNotAvailable). This can produce misleading diagnostics. Consider mapping AddrInUse to PortInUse and using OAuthCallbackError::Io (or a separate variant) for other bind errors.
Suggested change
| .map_err(|e| OAuthCallbackError::PortInUse(OAUTH_CALLBACK_PORT, e.to_string())) | |
| .map_err(|e| { | |
| if e.kind() == std::io::ErrorKind::AddrInUse { | |
| OAuthCallbackError::PortInUse(OAUTH_CALLBACK_PORT, e.to_string()) | |
| } else { | |
| OAuthCallbackError::Io(e.to_string()) | |
| } | |
| }) |
Member
Author
There was a problem hiding this comment.
Fixed in 050f3e7. The IPv4 fallback now checks e.kind() == AddrInUse specifically and maps other failures to OAuthCallbackError::Io instead of PortInUse.
…iority - Config::from_env() and Config::from_db() now call load_ironclaw_env() internally (after dotenvy::dotenv()), so CLI commands like `memory` and `config` correctly load DATABASE_URL from ~/.ironclaw/.env - Fix load order: standard ./.env first (higher priority), then ~/.ironclaw/.env, matching the documented priority chain - Collapse nested if/if-let into let-chains (clippy::collapsible_if) in oauth_defaults.rs, tool.rs, and secrets/store.rs - Fix rename_to_migrated to take &Path instead of &PathBuf Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Quote DATABASE_URL in .env writes so `#` in passwords isn't treated as a dotenv comment (e.g., `DATABASE_URL="postgres://..."`) - Add SSRF defenses to refresh_oauth_token(): require HTTPS, reject private/loopback IPs (with DNS resolution), disable redirects. token_url comes from tool capabilities JSON, so a malicious tool could otherwise exfiltrate refresh tokens. - Fix IPv4 bind error mapping: only map AddrInUse to PortInUse, use generic Io variant for other bind failures Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 37 out of 38 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| tokio::runtime::Builder::new_current_thread() | ||
| .enable_all() | ||
| .build() | ||
| .map_err(|e| format!("Failed to create HTTP runtime: {e}"))?, |
There was a problem hiding this comment.
The HTTP runtime is created with .new_current_thread() but without explicit .enable_io() or .enable_time(). While .enable_all() is called (which should enable both), this runtime lives inside a spawn_blocking thread that doesn't have access to the main runtime's I/O driver.
However, this pattern works because each current-thread runtime creates its own I/O resources. The real issue is that if the runtime creation fails or the I/O driver initialization fails, the error message "Failed to create HTTP runtime" doesn't distinguish between construction failure and driver issues, making debugging harder.
Consider adding more specific error context, especially for I/O driver failures which are common in containerized or restricted environments.
Suggested change
| .map_err(|e| format!("Failed to create HTTP runtime: {e}"))?, | |
| .map_err(|e| { | |
| // Provide more specific context for failures here, which are often | |
| // related to I/O or timer driver initialization in constrained | |
| // environments (e.g. containers with strict seccomp/apparmor). | |
| let kind_hint = match e.kind() { | |
| std::io::ErrorKind::PermissionDenied => { | |
| " (permission denied while initializing Tokio I/O driver; \ | |
| check container sandboxing / seccomp / AppArmor settings)" | |
| } | |
| std::io::ErrorKind::AddrInUse | std::io::ErrorKind::AddrNotAvailable => { | |
| " (network address issue while initializing Tokio I/O driver; \ | |
| check networking configuration and port availability)" | |
| } | |
| _ => " (failed while initializing Tokio I/O/timer drivers)", | |
| }; | |
| format!("Failed to create dedicated HTTP runtime{kind_hint}: {e}") | |
| })?, |
Comment on lines
248
to
309
| const SCHEMA: &str = r#"{ | ||
| "type": "object", | ||
| "required": ["action"], | ||
| "oneOf": [ | ||
| { | ||
| "properties": { | ||
| "action": { "const": "login" }, | ||
| "phone_number": { | ||
| "type": "string", | ||
| "description": "Phone number in international format (e.g., '+1234567890')" | ||
| } | ||
| }, | ||
| "required": ["action", "phone_number"] | ||
| "properties": { | ||
| "action": { | ||
| "type": "string", | ||
| "enum": ["login", "submit_auth_code", "submit_2fa_password", "get_me", "get_contacts", "get_chats", "get_messages", "send_message", "forward_message", "delete_message", "search_messages", "get_updates"], | ||
| "description": "The Telegram operation to perform" | ||
| }, | ||
| { | ||
| "properties": { | ||
| "action": { "const": "submit_auth_code" }, | ||
| "code": { | ||
| "type": "string", | ||
| "description": "Verification code received via SMS or Telegram" | ||
| } | ||
| }, | ||
| "required": ["action", "code"] | ||
| "phone_number": { | ||
| "type": "string", | ||
| "description": "Phone number in international format (e.g., '+1234567890'). Required for: login" | ||
| }, | ||
| { | ||
| "properties": { | ||
| "action": { "const": "submit_2fa_password" }, | ||
| "password": { | ||
| "type": "string", | ||
| "description": "Two-factor authentication password" | ||
| } | ||
| }, | ||
| "required": ["action", "password"] | ||
| "code": { | ||
| "type": "string", | ||
| "description": "Verification code received via SMS or Telegram. Required for: submit_auth_code" | ||
| }, | ||
| { | ||
| "properties": { | ||
| "action": { "const": "get_me" } | ||
| }, | ||
| "required": ["action"] | ||
| "password": { | ||
| "type": "string", | ||
| "description": "Two-factor authentication password. Required for: submit_2fa_password" | ||
| }, | ||
| { | ||
| "properties": { | ||
| "action": { "const": "get_contacts" } | ||
| }, | ||
| "required": ["action"] | ||
| "chat_id": { | ||
| "type": "integer", | ||
| "description": "Chat ID (negative for groups/channels). Required for: get_messages, send_message. Optional for: search_messages" | ||
| }, | ||
| { | ||
| "properties": { | ||
| "action": { "const": "get_chats" }, | ||
| "limit": { | ||
| "type": "integer", | ||
| "description": "Maximum number of chats to return (default: 20)", | ||
| "default": 20 | ||
| } | ||
| }, | ||
| "required": ["action"] | ||
| "limit": { | ||
| "type": "integer", | ||
| "description": "Maximum number of results (default: 20). Used by: get_chats, get_messages, search_messages", | ||
| "default": 20 | ||
| }, | ||
| { | ||
| "properties": { | ||
| "action": { "const": "get_messages" }, | ||
| "chat_id": { | ||
| "type": "integer", | ||
| "description": "Chat ID (negative for groups/channels)" | ||
| }, | ||
| "limit": { | ||
| "type": "integer", | ||
| "description": "Maximum number of messages (default: 20)", | ||
| "default": 20 | ||
| }, | ||
| "from_message_id": { | ||
| "type": "integer", | ||
| "description": "Start from this message ID for pagination" | ||
| } | ||
| }, | ||
| "required": ["action", "chat_id"] | ||
| "from_message_id": { | ||
| "type": "integer", | ||
| "description": "Start from this message ID for pagination. Used by: get_messages" | ||
| }, | ||
| { | ||
| "properties": { | ||
| "action": { "const": "send_message" }, | ||
| "chat_id": { | ||
| "type": "integer", | ||
| "description": "Chat ID to send the message to" | ||
| }, | ||
| "text": { | ||
| "type": "string", | ||
| "description": "Message text" | ||
| } | ||
| }, | ||
| "required": ["action", "chat_id", "text"] | ||
| "text": { | ||
| "type": "string", | ||
| "description": "Message text. Required for: send_message" | ||
| }, | ||
| { | ||
| "properties": { | ||
| "action": { "const": "forward_message" }, | ||
| "from_chat_id": { | ||
| "type": "integer", | ||
| "description": "Source chat ID" | ||
| }, | ||
| "to_chat_id": { | ||
| "type": "integer", | ||
| "description": "Destination chat ID" | ||
| }, | ||
| "message_ids": { | ||
| "type": "array", | ||
| "items": { "type": "integer" }, | ||
| "description": "Message IDs to forward" | ||
| } | ||
| }, | ||
| "required": ["action", "from_chat_id", "to_chat_id", "message_ids"] | ||
| "from_chat_id": { | ||
| "type": "integer", | ||
| "description": "Source chat ID. Required for: forward_message" | ||
| }, | ||
| { | ||
| "properties": { | ||
| "action": { "const": "delete_message" }, | ||
| "message_ids": { | ||
| "type": "array", | ||
| "items": { "type": "integer" }, | ||
| "description": "Message IDs to delete" | ||
| }, | ||
| "revoke": { | ||
| "type": "boolean", | ||
| "description": "Also delete for other participants (default: false)", | ||
| "default": false | ||
| } | ||
| }, | ||
| "required": ["action", "message_ids"] | ||
| "to_chat_id": { | ||
| "type": "integer", | ||
| "description": "Destination chat ID. Required for: forward_message" | ||
| }, | ||
| { | ||
| "properties": { | ||
| "action": { "const": "search_messages" }, | ||
| "query": { | ||
| "type": "string", | ||
| "description": "Search query" | ||
| }, | ||
| "chat_id": { | ||
| "type": "integer", | ||
| "description": "Chat ID to search within (omit for global search)" | ||
| }, | ||
| "limit": { | ||
| "type": "integer", | ||
| "description": "Maximum number of results (default: 20)", | ||
| "default": 20 | ||
| } | ||
| }, | ||
| "required": ["action", "query"] | ||
| "message_ids": { | ||
| "type": "array", | ||
| "items": { "type": "integer" }, | ||
| "description": "Message IDs. Required for: forward_message, delete_message" | ||
| }, | ||
| { | ||
| "properties": { | ||
| "action": { "const": "get_updates" } | ||
| }, | ||
| "required": ["action"] | ||
| "revoke": { | ||
| "type": "boolean", | ||
| "description": "Also delete for other participants (default: false). Used by: delete_message", | ||
| "default": false | ||
| }, | ||
| "query": { | ||
| "type": "string", | ||
| "description": "Search query. Required for: search_messages" | ||
| } | ||
| ] | ||
| } | ||
| }"#; |
There was a problem hiding this comment.
The new flat JSON schema structure (replacing oneOf) removes JSON Schema validation for required fields. While the Rust types use #[serde(tag = "action")] for parsing validation, LLMs and schema-driven clients may send incomplete requests that will only fail at runtime deserialization instead of failing JSON Schema validation upfront.
For example, the send_message action requires both chat_id and text, but the flat schema only documents this in the description field ("Required for: send_message"). A schema validator won't enforce this, potentially leading to confusing error messages when the Rust deserializer fails.
Consider either:
- Adding action-specific JSON Schema validation using
if/thenconditionals (supported in JSON Schema Draft 7+) - Adding a pre-validation step in the WASM wrapper that checks required fields per action before deserializing
- Documenting this as an intentional tradeoff (simpler schema, runtime validation only)
|
|
||
| # HTTP client | ||
| reqwest = { version = "0.12", default-features = false, features = ["json", "rustls-tls", "stream"] } | ||
| reqwest = { version = "0.12", default-features = false, features = ["json", "rustls-tls-native-roots", "stream"] } |
There was a problem hiding this comment.
Switching from rustls-tls to rustls-tls-native-roots changes certificate verification to use the OS-native certificate store instead of Mozilla's compiled-in root certificates. This can cause TLS failures on systems with outdated or misconfigured certificate stores (especially Windows/older Linux distributions).
While this gives better compatibility with corporate environments using custom CAs, it also introduces a new failure mode. Consider:
- Documenting this change in release notes as it may break environments that previously worked
- Adding fallback logic or a configuration option to switch between native-roots and webpki-roots
- Adding explicit error messages when TLS handshake fails that suggest checking system certificate store
Comment on lines
+89
to
+94
| pub fn save_database_url(url: &str) -> std::io::Result<()> { | ||
| let path = ironclaw_env_path(); | ||
| if let Some(parent) = path.parent() { | ||
| std::fs::create_dir_all(parent)?; | ||
| } | ||
| std::fs::write(&path, format!("DATABASE_URL=\"{}\"\n", url)) |
There was a problem hiding this comment.
The save_database_url function writes DATABASE_URL to disk without sanitizing or validating the input. If the URL contains literal double quotes, newlines, or other special characters, it could break the .env file format or introduce injection issues.
For example:
url = 'test"\nSECRET_KEY="hacked'would create a malformed .env with an injected lineurl = 'test\\"'would create an unclosed quote
Consider:
- Escaping double quotes in the URL value (replace
"with\\") - Rejecting URLs with newlines or other control characters
- Using a proper .env serialization library instead of format strings
serrrfirat
previously approved these changes
Feb 14, 2026
serrrfirat
left a comment
serrrfirat
left a comment
Collaborator
There was a problem hiding this comment.
All review comments addressed — XSS fix via html_escape(), Slack limit description cleaned up. LGTM.
Collaborator
|
Review findings (ordered by severity)
|
zmanian
reviewed
Feb 14, 2026
| } | ||
| } | ||
| Settings::load() | ||
| Settings::default() |
Collaborator
There was a problem hiding this comment.
Regression: load_settings says DB-or-disk fallback, but this branch returns Settings::default() when DB is unavailable. That makes config list/get show defaults instead of persisted local settings. Consider loading disk settings here (Settings::load()) for offline/no-DB behavior.
zmanian
reviewed
Feb 14, 2026
| } else { | ||
| settings.save()?; | ||
| } | ||
| let store = store.ok_or_else(|| { |
Collaborator
There was a problem hiding this comment.
Behavior regression: config set now hard-requires DB via store.ok_or_else(...). This removes no-DB/offline config updates. Consider preserving disk fallback when store is None.
zmanian
reviewed
Feb 14, 2026
| settings.reset(path).map_err(|e| anyhow::anyhow!("{}", e))?; | ||
| settings.save()?; | ||
| } | ||
| let store = store.ok_or_else(|| { |
Collaborator
There was a problem hiding this comment.
Behavior regression: config reset also hard-requires DB, same regression class as set, breaking reset in no-DB/offline flows. Consider restoring local fallback when DB is unavailable.
…-and-runtime # Conflicts: # src/llm/nearai.rs
Merged
jaswinder6991
pushed a commit
to jaswinder6991/ironclaw
that referenced
this pull request
Feb 26, 2026
* feat: Move debug log truncation from agent loop to REPL channel
Full tool output now flows through StatusUpdate so the web gateway
gets untruncated content. The REPL channel truncates at display time
(200 chars for tool results, thinking, and status messages).
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* fix: Flatten WASM tool schemas and fix host HTTP runtime contention
LLMs can't reliably follow oneOf + const discriminator patterns in JSON
Schema, causing tools like Google Calendar to receive malformed params
(e.g., {"operation":"list_events","data":{"calendarId":"primary"}} instead
of {"action":"list_events","calendar_id":"primary"}). Replace all 9 WASM
tool schemas with flat action enum + top-level properties. The serde
#[serde(tag = "action")] deserialization works identically.
Also fixes WASM host HTTP requests (channels and tools) stalling during
startup by replacing Handle::current().block_on() with a dedicated
single-threaded runtime per request, avoiding I/O driver contention.
Reduces verbose LLM debug logging (full request/response payloads) and
changes tower_http default from debug to warn.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* feat: Built-in OAuth credentials and combined Google scopes
Add infrastructure for shipping default OAuth credentials with the binary,
similar to how gcloud/rclone bake in their client_id. Credentials are set
at compile time via IRONCLAW_GOOGLE_CLIENT_ID / IRONCLAW_GOOGLE_CLIENT_SECRET
env vars, or can be hardcoded in src/cli/oauth_defaults.rs.
The fallback chain is: capabilities file > runtime env var > built-in defaults.
Also, when authing any Google tool, scopes from ALL installed Google tools
are now combined into a single OAuth request (they all share the same
google_oauth_token secret). One login covers Gmail, Calendar, Drive, etc.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* feat: Ship default Google OAuth credentials for zero-config auth
Google Desktop App credentials are not secret (per Google's own docs).
Hardcode them so `ironclaw tool auth <google-tool>` works out of the box
without requiring users to register their own OAuth app.
Credentials can still be overridden at compile time
(IRONCLAW_GOOGLE_CLIENT_ID) or runtime (GOOGLE_OAUTH_CLIENT_ID).
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* fix: Consistent OAuth callback port and polished landing page
- Use fixed port 9876 instead of scanning 9876-9886 (one redirect URI
to register in provider OAuth apps, deterministic behavior)
- Replace broken unicode checkmark with SVG icons (charset was missing,
rendered as mojibake)
- Dark themed landing page with proper card layout for both success
and error states
- Add charset=utf-8 to Content-Type headers
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* refactor: Unify OAuth callback server across all auth flows
All three OAuth flows (WASM tool auth, MCP server auth, NEAR AI login)
now share the same code from cli::oauth_defaults:
- Fixed port 9876 (one redirect URI to register per provider)
- Shared landing page HTML (dark card with SVG icons, proper charset)
- Parameterized wait_for_callback(listener, path, param, display_name)
Removes ~120 lines of duplicated callback/HTML code.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* Support for oauth token refresh
* refactor: Replace bootstrap.json with ~/.ironclaw/.env for DATABASE_URL
Kill the 4-field BootstrapConfig JSON file. Only DATABASE_URL actually
needs disk persistence (chicken-and-egg before DB connect). The other
three fields are now derived: pool_size defaults to 10 via env var,
secrets master key is auto-detected (env then keychain probe), and
onboard_completed is inferred from DATABASE_URL presence.
The new format is a standard .env file loaded via dotenvy early in
main, so DATABASE_URL is available as a regular env var everywhere.
Handles three upgrade paths:
- Clean start: wizard writes .env, reload after wizard completes
- Returning user: .env loaded at startup, business as usual
- Legacy upgrade: bootstrap.json auto-migrated to .env on first run
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* fix: Address PR review findings
- Fix UTF-8 panic in truncate_for_preview (byte-slice on char boundary)
- Cap WASM guest timeout_ms at 5 minutes to prevent resource exhaustion
- Fix localhost detection in requires_auth() to avoid substring matches
(e.g. "notlocalhost.com" no longer matches)
- Fix query param injection to insert before URL fragment
- Fix extract_host_from_url for IPv6 bracket notation
- Remove misleading schema defaults: Slack limit, Slides insertion_index,
Docs index (per-action defaults documented in descriptions instead)
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* style: Fix cargo fmt formatting
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* fix: IPv6 loopback support for OAuth listener and localhost detection
- bind_callback_listener: try [::1] first, fall back to 127.0.0.1,
so OAuth redirects work on systems where localhost resolves to ::1
- is_localhost_url: replace manual string parsing with url::Url for
correct handling of IPv6 brackets, ports, userinfo, etc.
- Add url crate as direct dependency (already a transitive dep)
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* fix: Address PR review feedback on runtime reuse, onboard check, and OAuth binding
- Remove session file check from check_onboard_needed(); DATABASE_URL is sufficient
- Detect AddrInUse on IPv6 bind and fail immediately instead of falling through to IPv4
- Reuse dedicated tokio runtime across HTTP calls in both tool and channel WASM wrappers
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* fix: HTML-escape provider name in OAuth landing page, simplify Slack limit description
- Add html_escape() to prevent XSS in landing_html() where provider_name
was interpolated directly into HTML (defense-in-depth, source is trusted
but escaping costs nothing)
- Remove per-action default numbers from Slack limit field description to
avoid confusing LLMs with conflicting defaults
Addresses review feedback from zmanian on PR nearai#42.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* fix: Save all bootstrap fields from wizard, fix config module comment
- Wizard now saves secrets_master_key_source and database_pool_size to
bootstrap.json (was only saving database_url and onboard_completed,
which broke secrets after fresh onboard since SecretsConfig::resolve
reads key source from bootstrap)
- Update config.rs module doc to reflect bootstrap.json priority chain
instead of the removed ~/.ironclaw/.env approach
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* refactor: Replace BootstrapConfig with .env-based bootstrap
DATABASE_URL is the only setting that needs disk persistence before
the database is available. Instead of a custom bootstrap.json with 4
fields, use a standard ~/.ironclaw/.env file loaded via dotenvy.
- Remove BootstrapConfig struct entirely
- Restore ironclaw_env_path(), load_ironclaw_env(), save_database_url()
- SecretsConfig::resolve() now auto-detects (env var then keychain probe)
instead of reading a saved source from bootstrap.json
- DatabaseConfig::resolve() reads DATABASE_URL from env only (dotenvy
loads ~/.ironclaw/.env into the environment early in startup)
- check_onboard_needed() is now sync (just checks env vars)
- Wizard save_and_summarize() works for both postgres and libsql backends
- One-time migration from bootstrap.json to .env preserved
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* fix: Ensure load_ironclaw_env() runs in all Config paths, fix .env priority
- Config::from_env() and Config::from_db() now call load_ironclaw_env()
internally (after dotenvy::dotenv()), so CLI commands like `memory`
and `config` correctly load DATABASE_URL from ~/.ironclaw/.env
- Fix load order: standard ./.env first (higher priority), then
~/.ironclaw/.env, matching the documented priority chain
- Collapse nested if/if-let into let-chains (clippy::collapsible_if)
in oauth_defaults.rs, tool.rs, and secrets/store.rs
- Fix rename_to_migrated to take &Path instead of &PathBuf
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* fix: Address PR review comments (quoting, SSRF, error mapping)
- Quote DATABASE_URL in .env writes so `#` in passwords isn't treated
as a dotenv comment (e.g., `DATABASE_URL="postgres://..."`)
- Add SSRF defenses to refresh_oauth_token(): require HTTPS, reject
private/loopback IPs (with DNS resolution), disable redirects.
token_url comes from tool capabilities JSON, so a malicious tool
could otherwise exfiltrate refresh tokens.
- Fix IPv4 bind error mapping: only map AddrInUse to PortInUse,
use generic Io variant for other bind failures
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 19, 2026
Load the actual compiled github WASM binary, send params with string-typed numbers through the coercion layer, and verify the WASM tool constructs correct HTTP API calls via a new HTTP interceptor in the WASM wrapper. Changes: - Add `http_interceptor` field to `StoreData` and `WasmToolWrapper` so WASM tool HTTP requests can be captured/mocked in tests - Make `prepare_tool_params` and `coercion` module public for integration tests - Add 3 e2e tests loading the real github WASM binary: - list_issues: `limit: "50"` → URL contains `per_page=50` - get_issue: `issue_number: "42"` → URL contains `/issues/42` - list_pull_requests: `limit: "25"` → URL contains `per_page=25` Tests gracefully skip if the WASM binary isn't compiled. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
ilblackdragon
added a commit
that referenced
this pull request
Mar 21, 2026
…1397) * fix: parameter coercion and validation for oneOf/anyOf/allOf schemas WASM extension tools with multi-action schemas (e.g. github extension) fail when the LLM passes numeric parameters as strings because the coercion layer skips JSON Schema combinators. This causes serde deserialization errors like `invalid type: string "100", expected u32`. Add discriminated-union resolution to the coercion layer: for oneOf/anyOf, match the active variant by const or single-element enum discriminators; for allOf, merge all variants' properties. Also propagate combinator awareness to schema validators, WASM wrapper helpers, and tool discovery so they no longer reject or ignore valid combinator-based schemas. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * test: add e2e tests for oneOf discriminated union parameter coercion Add three end-to-end tests using a fixture tool that mirrors the github WASM tool's oneOf schema with #[serde(tag = "action")] deserialization. Each test sends string-typed numeric/boolean params through the full agent loop, verifying that coercion resolves them before serde runs: - list_issues: limit "100" → 100 (integer in oneOf variant) - get_issue: issue_number "42" → 42 (integer in different variant) - create_pull_request: draft "true" → true (boolean in variant) Without the coercion fix these fail with: invalid type: string "100", expected u32 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * test: add real WASM github tool e2e tests with HTTP interception Load the actual compiled github WASM binary, send params with string-typed numbers through the coercion layer, and verify the WASM tool constructs correct HTTP API calls via a new HTTP interceptor in the WASM wrapper. Changes: - Add `http_interceptor` field to `StoreData` and `WasmToolWrapper` so WASM tool HTTP requests can be captured/mocked in tests - Make `prepare_tool_params` and `coercion` module public for integration tests - Add 3 e2e tests loading the real github WASM binary: - list_issues: `limit: "50"` → URL contains `per_page=50` - get_issue: `issue_number: "42"` → URL contains `/issues/42` - list_pull_requests: `limit: "25"` → URL contains `per_page=25` Tests gracefully skip if the WASM binary isn't compiled. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * refactor: simplify WASM e2e tests to use TestRig with with_wasm_tool() Replace the manual WasmToolWrapper construction with TestRig integration: - Add `with_wasm_tool(name, wasm_path, capabilities_path)` to TestRigBuilder that loads real WASM binaries and wires the shared HTTP interceptor - Build the HTTP interceptor before tool registration so it can be shared between AgentDeps and WASM tool wrappers - Rewrite github WASM e2e tests to use the standard trace pattern: TraceLlm sends tool calls with string params, http_exchanges specify expected outgoing requests and canned responses The test code is now identical to other trace-based e2e tests — no custom interceptors or manual WASM construction needed. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: address review comments on combinator schema support - Validate `has_combinators` checks array type (`.as_array().is_some()`) instead of bare `.is_some()` to reject malformed `{ "oneOf": {} }` - Validate top-level `required` keys against merged combinator variant properties when no top-level `properties` exists (both validators) - Deduplicate oneOf/anyOf handling into single loop in coercion.rs - Revert `pub mod coercion` to private; only re-export `prepare_tool_params` - Call `after_response` on interceptor after real HTTP when `before_request` returns None (recording mode correctness) - Fix formatting (CI failure) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: address second round of review comments - Fix headers deserialization bug: deserialize resp.headers_json as HashMap<String, String> then convert to Vec, not directly as Vec - Sort interceptor headers for deterministic trace fixtures - Update after_response comment: RecordingHttpInterceptor does exercise this path (returns None from before_request) - Mark WASM tests #[ignore] instead of silent skip — avoids false-green CI while keeping them runnable with --ignored - Fix with_wasm_tool signature: Option<PathBuf> instead of Option<impl Into<PathBuf>> which doesn't compile in nested position - Fix with_wasm_tool doc comment to match actual behavior - Revert prepare_tool_params to pub(crate) — no longer needed publicly Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: coerce empty strings to null for optional tool parameters LLMs often send "" instead of null/omitting optional parameters, causing parse errors in tools that expect typed values (e.g., timezone, schedule). PR #1127 fixed this per-field in the time tool. This commit adds dispatcher-level coercion so all tools benefit: - Non-required properties with value "" are coerced to null at the object level (based on the schema's `required` array) - Explicitly nullable schemas (`type: ["string", "null"]`) coerce "" to null in the per-value coercion path - Required string-only fields keep "" unchanged Closes #755 Co-Authored-By: spiritj <17498900+spiritj@users.noreply.github.com> Co-Authored-By: Xing Ji <41811005+micsama@users.noreply.github.com> Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * feat: complete coercion coverage for $ref, nested combinators, and additionalProperties Close remaining coercion gaps so 3rd-party tools (MCP servers, complex WASM tools) work correctly: - $ref resolution: inline all #/definitions/<name> and #/$defs/<name> references in a pre-pass before coercion, with depth limit (16) for circular ref safety - Nested combinators: resolve_effective_properties now recurses into variants that themselves contain allOf/oneOf/anyOf (depth limit 4) - additionalProperties inheritance: check allOf variants and matched oneOf/anyOf variant for additionalProperties schemas New tests: - resolves_ref_and_coerces_referenced_properties - resolves_nested_refs_in_oneof_variants - coerces_nested_combinators_allof_containing_oneof - coerces_array_items_with_oneof_discriminator - circular_ref_does_not_infinite_loop Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: address third round of review comments - Validators: tighten has_combinators to require at least one object-typed variant (has type:"object" or properties), rejecting non-object combinator schemas like { "oneOf": [{"type":"integer"}] } - Empty-string coercion: only coerce "" → null when schema allows null or doesn't allow string; pure type:"string" fields keep "" as meaningful - Fix comment: "coerce to null" → "return unchanged" for empty strings with no type match (code returns None, not null) - Redact credentials before passing to after_response interceptor to prevent secret leakage into recorded trace files - Switch to tokio::fs::read for async WASM binary loading in test rig - Add doc comment explaining soft URL check in WASM e2e tests Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * ci: retrigger after staging merge [skip-regression-check] * fix: merge staging, report non-array combinator values as errors Merge latest staging to fix CI (missing fallback_deliverable field). Add explicit error reporting when oneOf/anyOf/allOf values are not arrays in both strict and lenient validators. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: recurse into combinator variants that have properties but no explicit type Both validators only recursed into variants with `type: "object"`, missing variants that define `properties` without an explicit type (common in allOf patterns). Now recurse when variant has either. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Co-authored-by: spiritj <17498900+spiritj@users.noreply.github.com> Co-authored-by: Xing Ji <41811005+micsama@users.noreply.github.com>
tianhaoz95
pushed a commit
to tianhaoz95/clawgo
that referenced
this pull request
Mar 22, 2026
…earai#1397) * fix: parameter coercion and validation for oneOf/anyOf/allOf schemas WASM extension tools with multi-action schemas (e.g. github extension) fail when the LLM passes numeric parameters as strings because the coercion layer skips JSON Schema combinators. This causes serde deserialization errors like `invalid type: string "100", expected u32`. Add discriminated-union resolution to the coercion layer: for oneOf/anyOf, match the active variant by const or single-element enum discriminators; for allOf, merge all variants' properties. Also propagate combinator awareness to schema validators, WASM wrapper helpers, and tool discovery so they no longer reject or ignore valid combinator-based schemas. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * test: add e2e tests for oneOf discriminated union parameter coercion Add three end-to-end tests using a fixture tool that mirrors the github WASM tool's oneOf schema with #[serde(tag = "action")] deserialization. Each test sends string-typed numeric/boolean params through the full agent loop, verifying that coercion resolves them before serde runs: - list_issues: limit "100" → 100 (integer in oneOf variant) - get_issue: issue_number "42" → 42 (integer in different variant) - create_pull_request: draft "true" → true (boolean in variant) Without the coercion fix these fail with: invalid type: string "100", expected u32 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * test: add real WASM github tool e2e tests with HTTP interception Load the actual compiled github WASM binary, send params with string-typed numbers through the coercion layer, and verify the WASM tool constructs correct HTTP API calls via a new HTTP interceptor in the WASM wrapper. Changes: - Add `http_interceptor` field to `StoreData` and `WasmToolWrapper` so WASM tool HTTP requests can be captured/mocked in tests - Make `prepare_tool_params` and `coercion` module public for integration tests - Add 3 e2e tests loading the real github WASM binary: - list_issues: `limit: "50"` → URL contains `per_page=50` - get_issue: `issue_number: "42"` → URL contains `nearai/issues/42` - list_pull_requests: `limit: "25"` → URL contains `per_page=25` Tests gracefully skip if the WASM binary isn't compiled. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * refactor: simplify WASM e2e tests to use TestRig with with_wasm_tool() Replace the manual WasmToolWrapper construction with TestRig integration: - Add `with_wasm_tool(name, wasm_path, capabilities_path)` to TestRigBuilder that loads real WASM binaries and wires the shared HTTP interceptor - Build the HTTP interceptor before tool registration so it can be shared between AgentDeps and WASM tool wrappers - Rewrite github WASM e2e tests to use the standard trace pattern: TraceLlm sends tool calls with string params, http_exchanges specify expected outgoing requests and canned responses The test code is now identical to other trace-based e2e tests — no custom interceptors or manual WASM construction needed. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: address review comments on combinator schema support - Validate `has_combinators` checks array type (`.as_array().is_some()`) instead of bare `.is_some()` to reject malformed `{ "oneOf": {} }` - Validate top-level `required` keys against merged combinator variant properties when no top-level `properties` exists (both validators) - Deduplicate oneOf/anyOf handling into single loop in coercion.rs - Revert `pub mod coercion` to private; only re-export `prepare_tool_params` - Call `after_response` on interceptor after real HTTP when `before_request` returns None (recording mode correctness) - Fix formatting (CI failure) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: address second round of review comments - Fix headers deserialization bug: deserialize resp.headers_json as HashMap<String, String> then convert to Vec, not directly as Vec - Sort interceptor headers for deterministic trace fixtures - Update after_response comment: RecordingHttpInterceptor does exercise this path (returns None from before_request) - Mark WASM tests #[ignore] instead of silent skip — avoids false-green CI while keeping them runnable with --ignored - Fix with_wasm_tool signature: Option<PathBuf> instead of Option<impl Into<PathBuf>> which doesn't compile in nested position - Fix with_wasm_tool doc comment to match actual behavior - Revert prepare_tool_params to pub(crate) — no longer needed publicly Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: coerce empty strings to null for optional tool parameters LLMs often send "" instead of null/omitting optional parameters, causing parse errors in tools that expect typed values (e.g., timezone, schedule). PR nearai#1127 fixed this per-field in the time tool. This commit adds dispatcher-level coercion so all tools benefit: - Non-required properties with value "" are coerced to null at the object level (based on the schema's `required` array) - Explicitly nullable schemas (`type: ["string", "null"]`) coerce "" to null in the per-value coercion path - Required string-only fields keep "" unchanged Closes nearai#755 Co-Authored-By: spiritj <17498900+spiritj@users.noreply.github.com> Co-Authored-By: Xing Ji <41811005+micsama@users.noreply.github.com> Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * feat: complete coercion coverage for $ref, nested combinators, and additionalProperties Close remaining coercion gaps so 3rd-party tools (MCP servers, complex WASM tools) work correctly: - $ref resolution: inline all #/definitions/<name> and #/$defs/<name> references in a pre-pass before coercion, with depth limit (16) for circular ref safety - Nested combinators: resolve_effective_properties now recurses into variants that themselves contain allOf/oneOf/anyOf (depth limit 4) - additionalProperties inheritance: check allOf variants and matched oneOf/anyOf variant for additionalProperties schemas New tests: - resolves_ref_and_coerces_referenced_properties - resolves_nested_refs_in_oneof_variants - coerces_nested_combinators_allof_containing_oneof - coerces_array_items_with_oneof_discriminator - circular_ref_does_not_infinite_loop Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: address third round of review comments - Validators: tighten has_combinators to require at least one object-typed variant (has type:"object" or properties), rejecting non-object combinator schemas like { "oneOf": [{"type":"integer"}] } - Empty-string coercion: only coerce "" → null when schema allows null or doesn't allow string; pure type:"string" fields keep "" as meaningful - Fix comment: "coerce to null" → "return unchanged" for empty strings with no type match (code returns None, not null) - Redact credentials before passing to after_response interceptor to prevent secret leakage into recorded trace files - Switch to tokio::fs::read for async WASM binary loading in test rig - Add doc comment explaining soft URL check in WASM e2e tests Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * ci: retrigger after staging merge [skip-regression-check] * fix: merge staging, report non-array combinator values as errors Merge latest staging to fix CI (missing fallback_deliverable field). Add explicit error reporting when oneOf/anyOf/allOf values are not arrays in both strict and lenient validators. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: recurse into combinator variants that have properties but no explicit type Both validators only recursed into variants with `type: "object"`, missing variants that define `properties` without an explicit type (common in allOf patterns). Now recurse when variant has either. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Co-authored-by: spiritj <17498900+spiritj@users.noreply.github.com> Co-authored-by: Xing Ji <41811005+micsama@users.noreply.github.com>
bkutasi
pushed a commit
to bkutasi/ironclaw
that referenced
this pull request
Mar 28, 2026
* feat: Move debug log truncation from agent loop to REPL channel
Full tool output now flows through StatusUpdate so the web gateway
gets untruncated content. The REPL channel truncates at display time
(200 chars for tool results, thinking, and status messages).
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* fix: Flatten WASM tool schemas and fix host HTTP runtime contention
LLMs can't reliably follow oneOf + const discriminator patterns in JSON
Schema, causing tools like Google Calendar to receive malformed params
(e.g., {"operation":"list_events","data":{"calendarId":"primary"}} instead
of {"action":"list_events","calendar_id":"primary"}). Replace all 9 WASM
tool schemas with flat action enum + top-level properties. The serde
#[serde(tag = "action")] deserialization works identically.
Also fixes WASM host HTTP requests (channels and tools) stalling during
startup by replacing Handle::current().block_on() with a dedicated
single-threaded runtime per request, avoiding I/O driver contention.
Reduces verbose LLM debug logging (full request/response payloads) and
changes tower_http default from debug to warn.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* feat: Built-in OAuth credentials and combined Google scopes
Add infrastructure for shipping default OAuth credentials with the binary,
similar to how gcloud/rclone bake in their client_id. Credentials are set
at compile time via IRONCLAW_GOOGLE_CLIENT_ID / IRONCLAW_GOOGLE_CLIENT_SECRET
env vars, or can be hardcoded in src/cli/oauth_defaults.rs.
The fallback chain is: capabilities file > runtime env var > built-in defaults.
Also, when authing any Google tool, scopes from ALL installed Google tools
are now combined into a single OAuth request (they all share the same
google_oauth_token secret). One login covers Gmail, Calendar, Drive, etc.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* feat: Ship default Google OAuth credentials for zero-config auth
Google Desktop App credentials are not secret (per Google's own docs).
Hardcode them so `ironclaw tool auth <google-tool>` works out of the box
without requiring users to register their own OAuth app.
Credentials can still be overridden at compile time
(IRONCLAW_GOOGLE_CLIENT_ID) or runtime (GOOGLE_OAUTH_CLIENT_ID).
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* fix: Consistent OAuth callback port and polished landing page
- Use fixed port 9876 instead of scanning 9876-9886 (one redirect URI
to register in provider OAuth apps, deterministic behavior)
- Replace broken unicode checkmark with SVG icons (charset was missing,
rendered as mojibake)
- Dark themed landing page with proper card layout for both success
and error states
- Add charset=utf-8 to Content-Type headers
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* refactor: Unify OAuth callback server across all auth flows
All three OAuth flows (WASM tool auth, MCP server auth, NEAR AI login)
now share the same code from cli::oauth_defaults:
- Fixed port 9876 (one redirect URI to register per provider)
- Shared landing page HTML (dark card with SVG icons, proper charset)
- Parameterized wait_for_callback(listener, path, param, display_name)
Removes ~120 lines of duplicated callback/HTML code.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* Support for oauth token refresh
* refactor: Replace bootstrap.json with ~/.ironclaw/.env for DATABASE_URL
Kill the 4-field BootstrapConfig JSON file. Only DATABASE_URL actually
needs disk persistence (chicken-and-egg before DB connect). The other
three fields are now derived: pool_size defaults to 10 via env var,
secrets master key is auto-detected (env then keychain probe), and
onboard_completed is inferred from DATABASE_URL presence.
The new format is a standard .env file loaded via dotenvy early in
main, so DATABASE_URL is available as a regular env var everywhere.
Handles three upgrade paths:
- Clean start: wizard writes .env, reload after wizard completes
- Returning user: .env loaded at startup, business as usual
- Legacy upgrade: bootstrap.json auto-migrated to .env on first run
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* fix: Address PR review findings
- Fix UTF-8 panic in truncate_for_preview (byte-slice on char boundary)
- Cap WASM guest timeout_ms at 5 minutes to prevent resource exhaustion
- Fix localhost detection in requires_auth() to avoid substring matches
(e.g. "notlocalhost.com" no longer matches)
- Fix query param injection to insert before URL fragment
- Fix extract_host_from_url for IPv6 bracket notation
- Remove misleading schema defaults: Slack limit, Slides insertion_index,
Docs index (per-action defaults documented in descriptions instead)
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* style: Fix cargo fmt formatting
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* fix: IPv6 loopback support for OAuth listener and localhost detection
- bind_callback_listener: try [::1] first, fall back to 127.0.0.1,
so OAuth redirects work on systems where localhost resolves to ::1
- is_localhost_url: replace manual string parsing with url::Url for
correct handling of IPv6 brackets, ports, userinfo, etc.
- Add url crate as direct dependency (already a transitive dep)
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* fix: Address PR review feedback on runtime reuse, onboard check, and OAuth binding
- Remove session file check from check_onboard_needed(); DATABASE_URL is sufficient
- Detect AddrInUse on IPv6 bind and fail immediately instead of falling through to IPv4
- Reuse dedicated tokio runtime across HTTP calls in both tool and channel WASM wrappers
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* fix: HTML-escape provider name in OAuth landing page, simplify Slack limit description
- Add html_escape() to prevent XSS in landing_html() where provider_name
was interpolated directly into HTML (defense-in-depth, source is trusted
but escaping costs nothing)
- Remove per-action default numbers from Slack limit field description to
avoid confusing LLMs with conflicting defaults
Addresses review feedback from zmanian on PR nearai#42.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* fix: Save all bootstrap fields from wizard, fix config module comment
- Wizard now saves secrets_master_key_source and database_pool_size to
bootstrap.json (was only saving database_url and onboard_completed,
which broke secrets after fresh onboard since SecretsConfig::resolve
reads key source from bootstrap)
- Update config.rs module doc to reflect bootstrap.json priority chain
instead of the removed ~/.ironclaw/.env approach
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* refactor: Replace BootstrapConfig with .env-based bootstrap
DATABASE_URL is the only setting that needs disk persistence before
the database is available. Instead of a custom bootstrap.json with 4
fields, use a standard ~/.ironclaw/.env file loaded via dotenvy.
- Remove BootstrapConfig struct entirely
- Restore ironclaw_env_path(), load_ironclaw_env(), save_database_url()
- SecretsConfig::resolve() now auto-detects (env var then keychain probe)
instead of reading a saved source from bootstrap.json
- DatabaseConfig::resolve() reads DATABASE_URL from env only (dotenvy
loads ~/.ironclaw/.env into the environment early in startup)
- check_onboard_needed() is now sync (just checks env vars)
- Wizard save_and_summarize() works for both postgres and libsql backends
- One-time migration from bootstrap.json to .env preserved
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* fix: Ensure load_ironclaw_env() runs in all Config paths, fix .env priority
- Config::from_env() and Config::from_db() now call load_ironclaw_env()
internally (after dotenvy::dotenv()), so CLI commands like `memory`
and `config` correctly load DATABASE_URL from ~/.ironclaw/.env
- Fix load order: standard ./.env first (higher priority), then
~/.ironclaw/.env, matching the documented priority chain
- Collapse nested if/if-let into let-chains (clippy::collapsible_if)
in oauth_defaults.rs, tool.rs, and secrets/store.rs
- Fix rename_to_migrated to take &Path instead of &PathBuf
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* fix: Address PR review comments (quoting, SSRF, error mapping)
- Quote DATABASE_URL in .env writes so `#` in passwords isn't treated
as a dotenv comment (e.g., `DATABASE_URL="postgres://..."`)
- Add SSRF defenses to refresh_oauth_token(): require HTTPS, reject
private/loopback IPs (with DNS resolution), disable redirects.
token_url comes from tool capabilities JSON, so a malicious tool
could otherwise exfiltrate refresh tokens.
- Fix IPv4 bind error mapping: only map AddrInUse to PortInUse,
use generic Io variant for other bind failures
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
…earai#1397) * fix: parameter coercion and validation for oneOf/anyOf/allOf schemas WASM extension tools with multi-action schemas (e.g. github extension) fail when the LLM passes numeric parameters as strings because the coercion layer skips JSON Schema combinators. This causes serde deserialization errors like `invalid type: string "100", expected u32`. Add discriminated-union resolution to the coercion layer: for oneOf/anyOf, match the active variant by const or single-element enum discriminators; for allOf, merge all variants' properties. Also propagate combinator awareness to schema validators, WASM wrapper helpers, and tool discovery so they no longer reject or ignore valid combinator-based schemas. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * test: add e2e tests for oneOf discriminated union parameter coercion Add three end-to-end tests using a fixture tool that mirrors the github WASM tool's oneOf schema with #[serde(tag = "action")] deserialization. Each test sends string-typed numeric/boolean params through the full agent loop, verifying that coercion resolves them before serde runs: - list_issues: limit "100" → 100 (integer in oneOf variant) - get_issue: issue_number "42" → 42 (integer in different variant) - create_pull_request: draft "true" → true (boolean in variant) Without the coercion fix these fail with: invalid type: string "100", expected u32 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * test: add real WASM github tool e2e tests with HTTP interception Load the actual compiled github WASM binary, send params with string-typed numbers through the coercion layer, and verify the WASM tool constructs correct HTTP API calls via a new HTTP interceptor in the WASM wrapper. Changes: - Add `http_interceptor` field to `StoreData` and `WasmToolWrapper` so WASM tool HTTP requests can be captured/mocked in tests - Make `prepare_tool_params` and `coercion` module public for integration tests - Add 3 e2e tests loading the real github WASM binary: - list_issues: `limit: "50"` → URL contains `per_page=50` - get_issue: `issue_number: "42"` → URL contains `nearai/issues/42` - list_pull_requests: `limit: "25"` → URL contains `per_page=25` Tests gracefully skip if the WASM binary isn't compiled. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * refactor: simplify WASM e2e tests to use TestRig with with_wasm_tool() Replace the manual WasmToolWrapper construction with TestRig integration: - Add `with_wasm_tool(name, wasm_path, capabilities_path)` to TestRigBuilder that loads real WASM binaries and wires the shared HTTP interceptor - Build the HTTP interceptor before tool registration so it can be shared between AgentDeps and WASM tool wrappers - Rewrite github WASM e2e tests to use the standard trace pattern: TraceLlm sends tool calls with string params, http_exchanges specify expected outgoing requests and canned responses The test code is now identical to other trace-based e2e tests — no custom interceptors or manual WASM construction needed. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: address review comments on combinator schema support - Validate `has_combinators` checks array type (`.as_array().is_some()`) instead of bare `.is_some()` to reject malformed `{ "oneOf": {} }` - Validate top-level `required` keys against merged combinator variant properties when no top-level `properties` exists (both validators) - Deduplicate oneOf/anyOf handling into single loop in coercion.rs - Revert `pub mod coercion` to private; only re-export `prepare_tool_params` - Call `after_response` on interceptor after real HTTP when `before_request` returns None (recording mode correctness) - Fix formatting (CI failure) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: address second round of review comments - Fix headers deserialization bug: deserialize resp.headers_json as HashMap<String, String> then convert to Vec, not directly as Vec - Sort interceptor headers for deterministic trace fixtures - Update after_response comment: RecordingHttpInterceptor does exercise this path (returns None from before_request) - Mark WASM tests #[ignore] instead of silent skip — avoids false-green CI while keeping them runnable with --ignored - Fix with_wasm_tool signature: Option<PathBuf> instead of Option<impl Into<PathBuf>> which doesn't compile in nested position - Fix with_wasm_tool doc comment to match actual behavior - Revert prepare_tool_params to pub(crate) — no longer needed publicly Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: coerce empty strings to null for optional tool parameters LLMs often send "" instead of null/omitting optional parameters, causing parse errors in tools that expect typed values (e.g., timezone, schedule). PR nearai#1127 fixed this per-field in the time tool. This commit adds dispatcher-level coercion so all tools benefit: - Non-required properties with value "" are coerced to null at the object level (based on the schema's `required` array) - Explicitly nullable schemas (`type: ["string", "null"]`) coerce "" to null in the per-value coercion path - Required string-only fields keep "" unchanged Closes nearai#755 Co-Authored-By: spiritj <17498900+spiritj@users.noreply.github.com> Co-Authored-By: Xing Ji <41811005+micsama@users.noreply.github.com> Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * feat: complete coercion coverage for $ref, nested combinators, and additionalProperties Close remaining coercion gaps so 3rd-party tools (MCP servers, complex WASM tools) work correctly: - $ref resolution: inline all #/definitions/<name> and #/$defs/<name> references in a pre-pass before coercion, with depth limit (16) for circular ref safety - Nested combinators: resolve_effective_properties now recurses into variants that themselves contain allOf/oneOf/anyOf (depth limit 4) - additionalProperties inheritance: check allOf variants and matched oneOf/anyOf variant for additionalProperties schemas New tests: - resolves_ref_and_coerces_referenced_properties - resolves_nested_refs_in_oneof_variants - coerces_nested_combinators_allof_containing_oneof - coerces_array_items_with_oneof_discriminator - circular_ref_does_not_infinite_loop Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: address third round of review comments - Validators: tighten has_combinators to require at least one object-typed variant (has type:"object" or properties), rejecting non-object combinator schemas like { "oneOf": [{"type":"integer"}] } - Empty-string coercion: only coerce "" → null when schema allows null or doesn't allow string; pure type:"string" fields keep "" as meaningful - Fix comment: "coerce to null" → "return unchanged" for empty strings with no type match (code returns None, not null) - Redact credentials before passing to after_response interceptor to prevent secret leakage into recorded trace files - Switch to tokio::fs::read for async WASM binary loading in test rig - Add doc comment explaining soft URL check in WASM e2e tests Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * ci: retrigger after staging merge [skip-regression-check] * fix: merge staging, report non-array combinator values as errors Merge latest staging to fix CI (missing fallback_deliverable field). Add explicit error reporting when oneOf/anyOf/allOf values are not arrays in both strict and lenient validators. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: recurse into combinator variants that have properties but no explicit type Both validators only recursed into variants with `type: "object"`, missing variants that define `properties` without an explicit type (common in allOf patterns). Now recurse when variant has either. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Co-authored-by: spiritj <17498900+spiritj@users.noreply.github.com> Co-authored-by: Xing Ji <41811005+micsama@users.noreply.github.com>
drchirag1991
pushed a commit
to drchirag1991/ironclaw
that referenced
this pull request
Apr 8, 2026
…earai#1397) * fix: parameter coercion and validation for oneOf/anyOf/allOf schemas WASM extension tools with multi-action schemas (e.g. github extension) fail when the LLM passes numeric parameters as strings because the coercion layer skips JSON Schema combinators. This causes serde deserialization errors like `invalid type: string "100", expected u32`. Add discriminated-union resolution to the coercion layer: for oneOf/anyOf, match the active variant by const or single-element enum discriminators; for allOf, merge all variants' properties. Also propagate combinator awareness to schema validators, WASM wrapper helpers, and tool discovery so they no longer reject or ignore valid combinator-based schemas. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * test: add e2e tests for oneOf discriminated union parameter coercion Add three end-to-end tests using a fixture tool that mirrors the github WASM tool's oneOf schema with #[serde(tag = "action")] deserialization. Each test sends string-typed numeric/boolean params through the full agent loop, verifying that coercion resolves them before serde runs: - list_issues: limit "100" → 100 (integer in oneOf variant) - get_issue: issue_number "42" → 42 (integer in different variant) - create_pull_request: draft "true" → true (boolean in variant) Without the coercion fix these fail with: invalid type: string "100", expected u32 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * test: add real WASM github tool e2e tests with HTTP interception Load the actual compiled github WASM binary, send params with string-typed numbers through the coercion layer, and verify the WASM tool constructs correct HTTP API calls via a new HTTP interceptor in the WASM wrapper. Changes: - Add `http_interceptor` field to `StoreData` and `WasmToolWrapper` so WASM tool HTTP requests can be captured/mocked in tests - Make `prepare_tool_params` and `coercion` module public for integration tests - Add 3 e2e tests loading the real github WASM binary: - list_issues: `limit: "50"` → URL contains `per_page=50` - get_issue: `issue_number: "42"` → URL contains `nearai/issues/42` - list_pull_requests: `limit: "25"` → URL contains `per_page=25` Tests gracefully skip if the WASM binary isn't compiled. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * refactor: simplify WASM e2e tests to use TestRig with with_wasm_tool() Replace the manual WasmToolWrapper construction with TestRig integration: - Add `with_wasm_tool(name, wasm_path, capabilities_path)` to TestRigBuilder that loads real WASM binaries and wires the shared HTTP interceptor - Build the HTTP interceptor before tool registration so it can be shared between AgentDeps and WASM tool wrappers - Rewrite github WASM e2e tests to use the standard trace pattern: TraceLlm sends tool calls with string params, http_exchanges specify expected outgoing requests and canned responses The test code is now identical to other trace-based e2e tests — no custom interceptors or manual WASM construction needed. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: address review comments on combinator schema support - Validate `has_combinators` checks array type (`.as_array().is_some()`) instead of bare `.is_some()` to reject malformed `{ "oneOf": {} }` - Validate top-level `required` keys against merged combinator variant properties when no top-level `properties` exists (both validators) - Deduplicate oneOf/anyOf handling into single loop in coercion.rs - Revert `pub mod coercion` to private; only re-export `prepare_tool_params` - Call `after_response` on interceptor after real HTTP when `before_request` returns None (recording mode correctness) - Fix formatting (CI failure) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: address second round of review comments - Fix headers deserialization bug: deserialize resp.headers_json as HashMap<String, String> then convert to Vec, not directly as Vec - Sort interceptor headers for deterministic trace fixtures - Update after_response comment: RecordingHttpInterceptor does exercise this path (returns None from before_request) - Mark WASM tests #[ignore] instead of silent skip — avoids false-green CI while keeping them runnable with --ignored - Fix with_wasm_tool signature: Option<PathBuf> instead of Option<impl Into<PathBuf>> which doesn't compile in nested position - Fix with_wasm_tool doc comment to match actual behavior - Revert prepare_tool_params to pub(crate) — no longer needed publicly Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: coerce empty strings to null for optional tool parameters LLMs often send "" instead of null/omitting optional parameters, causing parse errors in tools that expect typed values (e.g., timezone, schedule). PR nearai#1127 fixed this per-field in the time tool. This commit adds dispatcher-level coercion so all tools benefit: - Non-required properties with value "" are coerced to null at the object level (based on the schema's `required` array) - Explicitly nullable schemas (`type: ["string", "null"]`) coerce "" to null in the per-value coercion path - Required string-only fields keep "" unchanged Closes nearai#755 Co-Authored-By: spiritj <17498900+spiritj@users.noreply.github.com> Co-Authored-By: Xing Ji <41811005+micsama@users.noreply.github.com> Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * feat: complete coercion coverage for $ref, nested combinators, and additionalProperties Close remaining coercion gaps so 3rd-party tools (MCP servers, complex WASM tools) work correctly: - $ref resolution: inline all #/definitions/<name> and #/$defs/<name> references in a pre-pass before coercion, with depth limit (16) for circular ref safety - Nested combinators: resolve_effective_properties now recurses into variants that themselves contain allOf/oneOf/anyOf (depth limit 4) - additionalProperties inheritance: check allOf variants and matched oneOf/anyOf variant for additionalProperties schemas New tests: - resolves_ref_and_coerces_referenced_properties - resolves_nested_refs_in_oneof_variants - coerces_nested_combinators_allof_containing_oneof - coerces_array_items_with_oneof_discriminator - circular_ref_does_not_infinite_loop Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: address third round of review comments - Validators: tighten has_combinators to require at least one object-typed variant (has type:"object" or properties), rejecting non-object combinator schemas like { "oneOf": [{"type":"integer"}] } - Empty-string coercion: only coerce "" → null when schema allows null or doesn't allow string; pure type:"string" fields keep "" as meaningful - Fix comment: "coerce to null" → "return unchanged" for empty strings with no type match (code returns None, not null) - Redact credentials before passing to after_response interceptor to prevent secret leakage into recorded trace files - Switch to tokio::fs::read for async WASM binary loading in test rig - Add doc comment explaining soft URL check in WASM e2e tests Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * ci: retrigger after staging merge [skip-regression-check] * fix: merge staging, report non-array combinator values as errors Merge latest staging to fix CI (missing fallback_deliverable field). Add explicit error reporting when oneOf/anyOf/allOf values are not arrays in both strict and lenient validators. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: recurse into combinator variants that have properties but no explicit type Both validators only recursed into variants with `type: "object"`, missing variants that define `properties` without an explicit type (common in allOf patterns). Now recurse when variant has either. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Co-authored-by: spiritj <17498900+spiritj@users.noreply.github.com> Co-authored-by: Xing Ji <41811005+micsama@users.noreply.github.com>
zmanian
added a commit
that referenced
this pull request
May 5, 2026
Closes the remaining libSQL/Postgres/JSONL gaps surfaced in the latest serrrfirat review pass on PR #3171. libSQL production gate (#34, #36, #41): - Reject `:memory:` for Production via the InMemory error variant. - Case-insensitive scheme matching for `HTTP://` / `HTTPS://` / `LibSQL://`, so a mixed-case URL no longer falls through to Builder::new_local and silently creates a node-local SQLite path. - Reject bare hostname-like values (e.g. `db.example.com`) for Production via a new ProductionLibsqlAmbiguousTarget error; LocalDev still accepts ergonomic forms like `events.db`. libSQL backend hardening (#45, #49, #50): - Skip `create_dir_all("")` for `events.db` in cwd. - Enable `journal_mode=WAL` + `synchronous=NORMAL` once at build time for file-backed local stores so a long replay reader can no longer block writer commits past `busy_timeout`. - Mirror the v1 retry pattern (`src/db/libsql/mod.rs::connect`): three attempts with exponential backoff so concurrent transient "unable to open database file" errors do not surface as durable-log failures. Postgres backend hardening (#46): - Honor `sslmode=require` for loopback configs so a TLS-only local Postgres / loopback proxy is accepted instead of forced to NoTls. JSONL hardening (#38, #40, #42, #43, #44): - Make JSONL constructors crate-private so production composition cannot bypass the single-node-durable acceptance gate. - Hash path components (SHA-256 / 16-hex-char prefix + 32-byte URL- encoded hint) so case-distinct IDs (`Alice` vs `alice`) cannot collide on case-insensitive filesystems and 256-byte scope IDs no longer overflow the 255-byte filename limit. - Snapshot file length before write/flush/sync; truncate back on any error so a partial write never leaves a torn JSON tail that wedges every subsequent append. - Create directories with `0o700` and stream files with `0o600` on Unix so durable history is not world-readable under the typical `umask 022`. SQL replay correctness (#37): - After fetching filtered rows, run a small unfiltered COUNT over the scanned cursor window in both libSQL and Postgres backends; mismatch surfaces a missing entry row as `EventError::ReplayGap`. JSONL already detects this via line-by-line cursor sequencing. Regression tests: - libSQL/Postgres production gate (case-insensitive scheme, in-memory, bare hostname). - Hashed-path-component case distinctness and length boundedness. - Atomic JSONL append: failed serialise leaves file at pre-append length and the next append still advances cursor cleanly. - Unix `0o700` JSONL root permissions. - libpq quoted single-quote socket path is local; whitespace-around- `=` keyword strings classify remote correctly. - libSQL replay surfaces a deleted entry row as `ReplayGap`. Deferred with explicit acknowledgement: - #39 (`ReadScope.invocation_id`) — module-doc note; needs a cross-crate change to `ironclaw_events` plus every replay caller. - #48 (replay holds writer-blocking locks) — inline note at the JSONL read path; needs a stream-bytes-snapshot redesign coordinated with the durable-log contract. Test plan - cargo test -p ironclaw_reborn_event_store - cargo test -p ironclaw_reborn_event_store --features "libsql postgres" - cargo test -p ironclaw_host_runtime - cargo test -p ironclaw_events - cargo test -p ironclaw_architecture reborn_crate_dependency_boundaries_hold - cargo clippy -p ironclaw_reborn_event_store --all-targets -- -D warnings - cargo clippy -p ironclaw_reborn_event_store --features "libsql postgres" --all-targets -- -D warnings - cargo clippy -p ironclaw_reborn_event_store --no-default-features --all-targets -- -D warnings - cargo clippy -p ironclaw_host_runtime --all-targets -- -D warnings - cargo fmt --all -- --check Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
serrrfirat
added a commit
that referenced
this pull request
May 6, 2026
* Add Reborn event store backends
* Address Reborn event store review feedback
* Address Reborn event store review feedback (round 2)
Postgres TLS / fail-closed (comment 3178548634): the Postgres event-store
client now uses tokio-postgres-rustls for any non-loopback URL, mirroring
src/db/tls.rs (native certs with webpki-roots fallback). Local sockets and
loopback hosts continue to use NoTls; unparseable URLs with a scheme are
treated as remote so a typo cannot silently downgrade to plaintext.
next_cursor advancement (comment 3178548646): JSONL, libSQL, and Postgres
read paths now track the highest scanned cursor independently from the last
matched cursor, and return max(last_matched, last_scanned). Without this,
a matched record followed by a filtered-out record would leave next_cursor
pinned to the matched cursor and the filtered record would be rescanned on
every subsequent replay.
JSONL bounded replay (comment 3178548670): replays now stream the JSONL
file line-by-line via BufReader, decoding only the cursor envelope until
a line crosses `after`, and stop as soon as `limit` matches are collected.
A `limit = 1` request against a multi-gigabyte stream no longer pays
full-file allocation or full-stream parse latency.
JSONL cross-process locking (comment 3178548701): JSONL appends now take
an OS-level exclusive advisory lock (std::fs::File::lock) for the entire
read-tail-cursor + write window. Two IronClaw processes pointing at the
same JSONL root will block on this lock and emit monotonically-sequenced
cursors instead of corrupting the stream with duplicates. Readers take a
shared lock to avoid observing partially-written tail lines.
New deps on the event-store crate (postgres feature only):
tokio-postgres-rustls, rustls, rustls-native-certs, webpki-roots, url.
All four were already used elsewhere in the workspace; no new external
crates are introduced. File locking uses stdlib (Rust 1.89+ stable).
Regression tests added:
- jsonl_replay_advances_next_cursor_past_trailing_filtered_records
- jsonl_concurrent_appenders_emit_monotonic_cursors_through_file_lock
- jsonl_bounded_replay_does_not_parse_the_whole_file
- postgres_store::tests::{local_postgres_urls_are_recognised,
remote_postgres_urls_require_tls,
unparseable_postgres_url_with_scheme_falls_closed_to_remote}
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(reborn_event_store): close TLS bypass for libpq keyword strings and http:// libsql in production
Two High-severity findings from PR #3171 review:
1. is_local_postgres_url() previously returned true for any string without
"://", so libpq keyword form like "host=db.example.com user=..." took
the NoTls branch and connected over plaintext. Walk the keyword list,
look at the actual host, and only treat localhost / socket paths as
local. Tests cover remote keyword strings, socket paths, and missing
host=.
2. Production profile previously accepted http:// libSQL URLs and forwarded
the auth token in cleartext. Reject http:// for RebornProfile::Production
with a typed error before the build call. LocalDev / Test still accept
http:// for running against local sqld instances. Tests cover both.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(reborn_event_store): use deadpool-postgres pool for transparent reconnection
Replaces the single Arc<Client> in PostgresStore with a deadpool_postgres::Pool.
The previous design built one tokio_postgres::Client at startup; if the
underlying connection task exited (idle timeout, server restart, failover,
transient network drop), all subsequent append/read operations would fail
forever because nothing rebuilt the connection.
deadpool's Manager replaces broken Clients on the next pool.get() call, so
each call site sees a live connection without per-site reconnect logic.
This mirrors v1's connection-management story (src/db/postgres.rs,
src/db/tls.rs both use deadpool-postgres) and avoids two divergent
reconnection paths in one binary.
Addresses serrrfirat's Medium-severity finding on PR #3171.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* chore(tests): fix post-merge clippy warning
* fix(reborn_event_store): close TLS-mode + libpq-config + JSONL fsync gaps
Three follow-ups from PR #3171 round-3 review:
1. CRITICAL — remote Postgres TLS not actually enforced. Passing a rustls
connector is not enough on its own: tokio-postgres only consults the
connector when Config::ssl_mode is Prefer or Require, and `sslmode=disable`
in the connection string returns a plaintext stream before TLS is
attempted. Add `enforce_remote_ssl_mode`: reject Disable for non-local
configs, and force Prefer (the default) up to Require so the server
cannot decline TLS without failing the connection.
2. HIGH — local/remote detector missed valid libpq forms. Re-parsing the
raw connection string failed on `hostaddr=10.0.0.5` (numeric-IP keyword,
no `host=` entry), `postgresql:///db?host=db.example.com` (URL with
empty authority + host in query), and `host=/var/run/postgresql,
db.example.com` (mixed socket/TCP list). Replace with
`is_local_postgres_config` that walks the parsed `Config::get_hosts()`
and `Config::get_hostaddrs()` so all libpq normalisations land in the
same code path.
3. MEDIUM — JSONL append fsynced file contents but not the parent
directory entry. On POSIX the new file's directory entry must also be
fsynced for crash durability; without it the first append can vanish
after a power loss even though `append()` returned success. Detect
first-create via `path.exists()` pre-open and fsync the parent dir
after `sync_data()`.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(reborn_event_store): address PR #3171 round-3 review findings
Closes the remaining libSQL/Postgres/JSONL gaps surfaced in the latest
serrrfirat review pass on PR #3171.
libSQL production gate (#34, #36, #41):
- Reject `:memory:` for Production via the InMemory error variant.
- Case-insensitive scheme matching for `HTTP://` / `HTTPS://` /
`LibSQL://`, so a mixed-case URL no longer falls through to
Builder::new_local and silently creates a node-local SQLite path.
- Reject bare hostname-like values (e.g. `db.example.com`) for
Production via a new ProductionLibsqlAmbiguousTarget error; LocalDev
still accepts ergonomic forms like `events.db`.
libSQL backend hardening (#45, #49, #50):
- Skip `create_dir_all("")` for `events.db` in cwd.
- Enable `journal_mode=WAL` + `synchronous=NORMAL` once at build time
for file-backed local stores so a long replay reader can no longer
block writer commits past `busy_timeout`.
- Mirror the v1 retry pattern (`src/db/libsql/mod.rs::connect`): three
attempts with exponential backoff so concurrent transient
"unable to open database file" errors do not surface as durable-log
failures.
Postgres backend hardening (#46):
- Honor `sslmode=require` for loopback configs so a TLS-only local
Postgres / loopback proxy is accepted instead of forced to NoTls.
JSONL hardening (#38, #40, #42, #43, #44):
- Make JSONL constructors crate-private so production composition
cannot bypass the single-node-durable acceptance gate.
- Hash path components (SHA-256 / 16-hex-char prefix + 32-byte URL-
encoded hint) so case-distinct IDs (`Alice` vs `alice`) cannot
collide on case-insensitive filesystems and 256-byte scope IDs no
longer overflow the 255-byte filename limit.
- Snapshot file length before write/flush/sync; truncate back on any
error so a partial write never leaves a torn JSON tail that wedges
every subsequent append.
- Create directories with `0o700` and stream files with `0o600` on
Unix so durable history is not world-readable under the typical
`umask 022`.
SQL replay correctness (#37):
- After fetching filtered rows, run a small unfiltered COUNT over the
scanned cursor window in both libSQL and Postgres backends; mismatch
surfaces a missing entry row as `EventError::ReplayGap`. JSONL
already detects this via line-by-line cursor sequencing.
Regression tests:
- libSQL/Postgres production gate (case-insensitive scheme, in-memory,
bare hostname).
- Hashed-path-component case distinctness and length boundedness.
- Atomic JSONL append: failed serialise leaves file at pre-append
length and the next append still advances cursor cleanly.
- Unix `0o700` JSONL root permissions.
- libpq quoted single-quote socket path is local; whitespace-around-
`=` keyword strings classify remote correctly.
- libSQL replay surfaces a deleted entry row as `ReplayGap`.
Deferred with explicit acknowledgement:
- #39 (`ReadScope.invocation_id`) — module-doc note; needs a
cross-crate change to `ironclaw_events` plus every replay caller.
- #48 (replay holds writer-blocking locks) — inline note at the JSONL
read path; needs a stream-bytes-snapshot redesign coordinated with
the durable-log contract.
Test plan
- cargo test -p ironclaw_reborn_event_store
- cargo test -p ironclaw_reborn_event_store --features "libsql postgres"
- cargo test -p ironclaw_host_runtime
- cargo test -p ironclaw_events
- cargo test -p ironclaw_architecture reborn_crate_dependency_boundaries_hold
- cargo clippy -p ironclaw_reborn_event_store --all-targets -- -D warnings
- cargo clippy -p ironclaw_reborn_event_store --features "libsql postgres" --all-targets -- -D warnings
- cargo clippy -p ironclaw_reborn_event_store --no-default-features --all-targets -- -D warnings
- cargo clippy -p ironclaw_host_runtime --all-targets -- -D warnings
- cargo fmt --all -- --check
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-authored-by: serrrfirat <f@nuff.tech>
theredspoon
referenced
this pull request
in theredspoon/ironclaw
Jun 21, 2026
* Add Reborn event store backends
* Address Reborn event store review feedback
* Address Reborn event store review feedback (round 2)
Postgres TLS / fail-closed (comment 3178548634): the Postgres event-store
client now uses tokio-postgres-rustls for any non-loopback URL, mirroring
src/db/tls.rs (native certs with webpki-roots fallback). Local sockets and
loopback hosts continue to use NoTls; unparseable URLs with a scheme are
treated as remote so a typo cannot silently downgrade to plaintext.
next_cursor advancement (comment 3178548646): JSONL, libSQL, and Postgres
read paths now track the highest scanned cursor independently from the last
matched cursor, and return max(last_matched, last_scanned). Without this,
a matched record followed by a filtered-out record would leave next_cursor
pinned to the matched cursor and the filtered record would be rescanned on
every subsequent replay.
JSONL bounded replay (comment 3178548670): replays now stream the JSONL
file line-by-line via BufReader, decoding only the cursor envelope until
a line crosses `after`, and stop as soon as `limit` matches are collected.
A `limit = 1` request against a multi-gigabyte stream no longer pays
full-file allocation or full-stream parse latency.
JSONL cross-process locking (comment 3178548701): JSONL appends now take
an OS-level exclusive advisory lock (std::fs::File::lock) for the entire
read-tail-cursor + write window. Two IronClaw processes pointing at the
same JSONL root will block on this lock and emit monotonically-sequenced
cursors instead of corrupting the stream with duplicates. Readers take a
shared lock to avoid observing partially-written tail lines.
New deps on the event-store crate (postgres feature only):
tokio-postgres-rustls, rustls, rustls-native-certs, webpki-roots, url.
All four were already used elsewhere in the workspace; no new external
crates are introduced. File locking uses stdlib (Rust 1.89+ stable).
Regression tests added:
- jsonl_replay_advances_next_cursor_past_trailing_filtered_records
- jsonl_concurrent_appenders_emit_monotonic_cursors_through_file_lock
- jsonl_bounded_replay_does_not_parse_the_whole_file
- postgres_store::tests::{local_postgres_urls_are_recognised,
remote_postgres_urls_require_tls,
unparseable_postgres_url_with_scheme_falls_closed_to_remote}
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(reborn_event_store): close TLS bypass for libpq keyword strings and http:// libsql in production
Two High-severity findings from PR nearai#3171 review:
1. is_local_postgres_url() previously returned true for any string without
"://", so libpq keyword form like "host=db.example.com user=..." took
the NoTls branch and connected over plaintext. Walk the keyword list,
look at the actual host, and only treat localhost / socket paths as
local. Tests cover remote keyword strings, socket paths, and missing
host=.
2. Production profile previously accepted http:// libSQL URLs and forwarded
the auth token in cleartext. Reject http:// for RebornProfile::Production
with a typed error before the build call. LocalDev / Test still accept
http:// for running against local sqld instances. Tests cover both.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(reborn_event_store): use deadpool-postgres pool for transparent reconnection
Replaces the single Arc<Client> in PostgresStore with a deadpool_postgres::Pool.
The previous design built one tokio_postgres::Client at startup; if the
underlying connection task exited (idle timeout, server restart, failover,
transient network drop), all subsequent append/read operations would fail
forever because nothing rebuilt the connection.
deadpool's Manager replaces broken Clients on the next pool.get() call, so
each call site sees a live connection without per-site reconnect logic.
This mirrors v1's connection-management story (src/db/postgres.rs,
src/db/tls.rs both use deadpool-postgres) and avoids two divergent
reconnection paths in one binary.
Addresses serrrfirat's Medium-severity finding on PR nearai#3171.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* chore(tests): fix post-merge clippy warning
* fix(reborn_event_store): close TLS-mode + libpq-config + JSONL fsync gaps
Three follow-ups from PR nearai#3171 round-3 review:
1. CRITICAL — remote Postgres TLS not actually enforced. Passing a rustls
connector is not enough on its own: tokio-postgres only consults the
connector when Config::ssl_mode is Prefer or Require, and `sslmode=disable`
in the connection string returns a plaintext stream before TLS is
attempted. Add `enforce_remote_ssl_mode`: reject Disable for non-local
configs, and force Prefer (the default) up to Require so the server
cannot decline TLS without failing the connection.
2. HIGH — local/remote detector missed valid libpq forms. Re-parsing the
raw connection string failed on `hostaddr=10.0.0.5` (numeric-IP keyword,
no `host=` entry), `postgresql:///db?host=db.example.com` (URL with
empty authority + host in query), and `host=/var/run/postgresql,
db.example.com` (mixed socket/TCP list). Replace with
`is_local_postgres_config` that walks the parsed `Config::get_hosts()`
and `Config::get_hostaddrs()` so all libpq normalisations land in the
same code path.
3. MEDIUM — JSONL append fsynced file contents but not the parent
directory entry. On POSIX the new file's directory entry must also be
fsynced for crash durability; without it the first append can vanish
after a power loss even though `append()` returned success. Detect
first-create via `path.exists()` pre-open and fsync the parent dir
after `sync_data()`.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(reborn_event_store): address PR nearai#3171 round-3 review findings
Closes the remaining libSQL/Postgres/JSONL gaps surfaced in the latest
serrrfirat review pass on PR nearai#3171.
libSQL production gate (#34, #36, #41):
- Reject `:memory:` for Production via the InMemory error variant.
- Case-insensitive scheme matching for `HTTP://` / `HTTPS://` /
`LibSQL://`, so a mixed-case URL no longer falls through to
Builder::new_local and silently creates a node-local SQLite path.
- Reject bare hostname-like values (e.g. `db.example.com`) for
Production via a new ProductionLibsqlAmbiguousTarget error; LocalDev
still accepts ergonomic forms like `events.db`.
libSQL backend hardening (#45, #49, #50):
- Skip `create_dir_all("")` for `events.db` in cwd.
- Enable `journal_mode=WAL` + `synchronous=NORMAL` once at build time
for file-backed local stores so a long replay reader can no longer
block writer commits past `busy_timeout`.
- Mirror the v1 retry pattern (`src/db/libsql/mod.rs::connect`): three
attempts with exponential backoff so concurrent transient
"unable to open database file" errors do not surface as durable-log
failures.
Postgres backend hardening (#46):
- Honor `sslmode=require` for loopback configs so a TLS-only local
Postgres / loopback proxy is accepted instead of forced to NoTls.
JSONL hardening (#38, #40, #42, #43, #44):
- Make JSONL constructors crate-private so production composition
cannot bypass the single-node-durable acceptance gate.
- Hash path components (SHA-256 / 16-hex-char prefix + 32-byte URL-
encoded hint) so case-distinct IDs (`Alice` vs `alice`) cannot
collide on case-insensitive filesystems and 256-byte scope IDs no
longer overflow the 255-byte filename limit.
- Snapshot file length before write/flush/sync; truncate back on any
error so a partial write never leaves a torn JSON tail that wedges
every subsequent append.
- Create directories with `0o700` and stream files with `0o600` on
Unix so durable history is not world-readable under the typical
`umask 022`.
SQL replay correctness (#37):
- After fetching filtered rows, run a small unfiltered COUNT over the
scanned cursor window in both libSQL and Postgres backends; mismatch
surfaces a missing entry row as `EventError::ReplayGap`. JSONL
already detects this via line-by-line cursor sequencing.
Regression tests:
- libSQL/Postgres production gate (case-insensitive scheme, in-memory,
bare hostname).
- Hashed-path-component case distinctness and length boundedness.
- Atomic JSONL append: failed serialise leaves file at pre-append
length and the next append still advances cursor cleanly.
- Unix `0o700` JSONL root permissions.
- libpq quoted single-quote socket path is local; whitespace-around-
`=` keyword strings classify remote correctly.
- libSQL replay surfaces a deleted entry row as `ReplayGap`.
Deferred with explicit acknowledgement:
- #39 (`ReadScope.invocation_id`) — module-doc note; needs a
cross-crate change to `ironclaw_events` plus every replay caller.
- #48 (replay holds writer-blocking locks) — inline note at the JSONL
read path; needs a stream-bytes-snapshot redesign coordinated with
the durable-log contract.
Test plan
- cargo test -p ironclaw_reborn_event_store
- cargo test -p ironclaw_reborn_event_store --features "libsql postgres"
- cargo test -p ironclaw_host_runtime
- cargo test -p ironclaw_events
- cargo test -p ironclaw_architecture reborn_crate_dependency_boundaries_hold
- cargo clippy -p ironclaw_reborn_event_store --all-targets -- -D warnings
- cargo clippy -p ironclaw_reborn_event_store --features "libsql postgres" --all-targets -- -D warnings
- cargo clippy -p ironclaw_reborn_event_store --no-default-features --all-targets -- -D warnings
- cargo clippy -p ironclaw_host_runtime --all-targets -- -D warnings
- cargo fmt --all -- --check
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-authored-by: serrrfirat <f@nuff.tech>
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.
This got a bunch of other changes pulled in to make sure settings are properly passed and oauth tokens are rotated to run end to end wasm tools.