diff --git a/docs/cli-installer.md b/docs/cli-installer.md index e4ec42b4..cd97e030 100644 --- a/docs/cli-installer.md +++ b/docs/cli-installer.md @@ -27,11 +27,12 @@ ClaudeAgentSDK::CLIInstaller.installed_path `install` is idempotent and safe to run concurrently, so it fits `bin/setup`, a cached Docker layer, and every process of a multi-process boot: - The install directory's `VERSION` file records the installed version, verified SHA-256, and target platform (OS, architecture, libc). The shortcut re-hashes the vendored binary (~0.1s for the real 245MB binary) and only skips the download when all three match — a truncated binary or a cache copied from another platform is reinstalled instead of trusted. It makes **no network request**, so same-platform repeat boots work offline — with a pinned concrete version; `'stable'`/`'latest'` must always re-resolve through the endpoint, which is one more reason to pin in production. Older one- or two-line metadata lacks a platform and requires one online reinstall to migrate; subsequent pinned installs work offline again. -- An exclusive `flock` on `/.install.lock` covers the whole check → download → place → record sequence, so parallel installs into one directory don't race; the loser simply observes the finished install. +- An exclusive `flock` on `/.install.lock` covers the whole check → download → record → place sequence, so parallel installs into one directory don't race — whether they come from separate processes, threads, or fibers of one `Async` reactor (a waiting installer polls the lock rather than blocking its thread on it); the loser simply observes the finished install. +- The shortcut also works where the running process cannot write the install directory: an image built as root and run as another user, or a read-only root filesystem. `install` cannot open its lock file there (`EACCES`, `EROFS` or `EPERM`), and still returns the binary when the request is a concrete version that is already installed and intact — the same `VERSION` and SHA-256 check, without the lock. Anything that would need a write raises `CLIInstallError` as before: a dist-tag (`'stable'` / `'latest'`, which has to be resolved and may have to be installed), a different version, a damaged binary, an empty directory. Failures (unsupported platform, invalid version, HTTP error, response-size cap, oversized download, checksum mismatch, filesystem errors) raise `ClaudeAgentSDK::CLIInstallError`. -**A failed install never breaks a working one.** The new binary is downloaded to a temp file, checksum-verified and recorded, and only then renamed into place — the rename is the last step, and nothing can fail after it. So a failed upgrade leaves the previously installed binary intact and runnable (the SDK keeps working), and the next `install` redoes it cleanly. A first install that fails leaves nothing behind at all. +**A failed install never breaks a working one.** The new binary is downloaded to a temp file, checksum-verified and recorded, and only then renamed into place — the rename is the last step, and nothing can fail after it. So a failed upgrade leaves the previously installed binary intact and runnable (the SDK keeps working), and the next `install` redoes it cleanly. The binary and `VERSION` are also flushed to disk (`fsync`) before they are renamed into place, so a machine that loses power right after an install comes back with a complete binary or with the previous state, not with a truncated file under the published name. A first install that fails never publishes a binary. It can leave the install directory and its `.install.lock` behind and — when the step that failed is the last one, the rename — a `VERSION` that already records the version. Nothing trusts that file on its own: discovery finds no binary there and moves on, and the next `install` redoes it cleanly. ## When the pin moves @@ -96,6 +97,17 @@ RUN bin/rails claude_agent_sdk:install_cli The variable is deliberately not rake's conventional `VERSION`, which Rails' `db:migrate` uses and build environments often export for an app version or git SHA. An empty `CLAUDE_CLI_VERSION` means the gem's pin. +## Proxies and custom CAs + +The installer downloads over HTTPS with Ruby's `Net::HTTP` and reads the usual environment variables: + +- **`HTTPS_PROXY`** (or `https_proxy`) names the proxy to download through, as an `http://` URL: `HTTPS_PROXY=http://proxy.corp.example:3128`, with `user:password@` in front of the host for an authenticating proxy (percent-encode special characters). The download is tunnelled through it with `CONNECT`, so TLS still ends at the release endpoint. +- **`NO_PROXY`** (or `no_proxy`) lists the hosts, domain suffixes and IP ranges to reach directly, separated by commas. +- With neither spelling of `HTTPS_PROXY` set, `Net::HTTP`'s own default applies: it goes through `http_proxy` when that is set. +- **`SSL_CERT_FILE`** points OpenSSL at another CA bundle, which is what a TLS-inspecting proxy needs. Set it in the environment the process starts with. The server certificate is always verified; there is no switch to turn that off. + +This is not everything `curl` understands. `ALL_PROXY` is not read, and only an `http://` proxy URL is used: a value without a scheme (`proxy.corp.example:3128`), a `socks5://` proxy or an `https://` one is ignored. + ## Supported platforms `darwin-arm64`, `darwin-x64` (Rosetta 2 gets the arm64 build), `linux-x64`, `linux-arm64`, and the `-musl` variants. Windows is not supported. diff --git a/docs/mcp-servers.md b/docs/mcp-servers.md index 296cbb6b..170aba49 100644 --- a/docs/mcp-servers.md +++ b/docs/mcp-servers.md @@ -56,7 +56,28 @@ ClaudeAgentSDK.create_tool('lookup_order', 'Look up an order', { id: :string }) end ``` -Any other return value (`nil`, an Integer, an Array, ...) is reported to Claude as an `isError: true` result saying the tool must return a hash with a `:content` key. +Any other return value (`nil`, an Integer, an Array, ...) is reported to Claude as an `isError: true` result saying the tool must return a hash with a `:content` key. So is a Hash whose `:content` is not an Array — `{ content: 'text' }`, or a single block Hash — with a message saying `:content` must be an Array of content blocks; return the String itself, or wrap the block in an Array. + +## Shorthand Input Schemas + +`{ name: :string }` is shorthand for a JSON Schema object in which every listed parameter is required. Each value names a type, as a Ruby class or as a Symbol: + +| Shorthand | JSON Schema type | The handler receives | +| --- | --- | --- | +| `String`, `:string` | `string` | a String | +| `Integer`, `:integer` | `integer` | an Integer | +| `Float`, `:float`, `:number` | `number` | an Integer or a Float | +| `TrueClass`, `FalseClass`, `:boolean` | `boolean` | `true` or `false` | +| `Array`, `:array` | `array` | an Array, with elements of any type | +| `Hash`, `:object` | `object` | a Hash with Symbol keys | + +```ruby +ClaudeAgentSDK.create_tool('tag_order', 'Tag an order', { order_id: Integer, tags: Array }) do |args| + "Tagged order #{args[:order_id]} with #{args[:tags].join(', ')}" +end +``` + +Arguments are validated against these types before the handler runs. For an optional parameter, the type of an Array's elements, an enum or a per-parameter description, pass a full JSON Schema instead (next section). ## Pre-built JSON Schemas diff --git a/formal/tla/CLIInstaller.tla b/formal/tla/CLIInstaller.tla index 86ccfdf4..5fe752bf 100644 --- a/formal/tla/CLIInstaller.tla +++ b/formal/tla/CLIInstaller.tla @@ -183,7 +183,27 @@ Fail(i) == /\ Leave(i, "fail") /\ UNCHANGED <> -\* SIGKILL / OOM / power loss: no `ensure`, the temp file stays behind. +\* The process dies with no `ensure`: SIGKILL, OOM, or the machine losing +\* power. The temp file stays behind and the flock is gone (released by the +\* kernel, or lost with it). +\* +\* UNCHANGED is exact for a dead process: the files are what the completed +\* steps left. For a power loss it holds for the two files the properties +\* read, because of what the code fsyncs: the downloaded bytes, and the +\* binary's mode after its chmod, before Rename, and the VERSION bytes before +\* Record's rename. A name that survives the +\* outage therefore holds a complete file -- `binary` and `vfile` are each the +\* old value or the new one, never an empty or partial file. (Before those +\* fsyncs a first install could come back as an empty executable `claude` +\* beside a VERSION naming it, which breaks ReadersSeeVerifiedBinary.) +\* A rename that had not reached the disk is undone; on a filesystem that +\* commits directory operations in order, that is this step at an earlier pc. +\* +\* Not modelled: the directory fsync after Rename, which is what keeps a +\* FINISHED install across an outage (best-effort; skipped where a directory +\* cannot be synced), and `tmps` after an outage -- a leftover temp file may be +\* missing or short, which is harmless because temp files are only ever +\* swept, never read. All of it assumes fsync(2) reaches stable storage. Crash(i) == /\ pc[i] # "idle" /\ Leave(i, "crash") diff --git a/formal/tla/README.md b/formal/tla/README.md index 0b2ad1d5..a3f7bb02 100644 --- a/formal/tla/README.md +++ b/formal/tla/README.md @@ -45,5 +45,7 @@ There are three properties. `EverySenderFinishes` says every control method retu - File contents are abstracted to "the version they are the bytes of", so a checksum match is equality. Platform detection is not modelled. - The models assume that POSIX `rename` is atomic and that the kernel releases a dead process's flock. `ReadersSeeVerifiedBinary` holds by construction given those assumptions. +- Durability is assumed, not modelled. `Crash` leaves the files as the completed steps left them, which is exact when a process dies. Across a power loss it rests on the installer's `fsync`s — the downloaded bytes before `Rename`, the `VERSION` bytes before `Record`'s rename, and (best-effort) the directory after `Rename` — and on `fsync(2)` reaching stable storage: a surviving name then holds a complete file, and a rename that had not reached the disk is undone, which is `Crash` at an earlier step on a filesystem that commits directory operations in order. +- Not modelled: `install`'s read-only shortcut. When the lock file cannot be opened (`EACCES`, `EROFS`, `EPERM`) and the request is a concrete version that is already installed and re-hashes to its recorded checksum, `install` returns the binary without the lock. That path takes no lock and writes nothing, so it is one more lock-free reader, which `ReadersSeeVerifiedBinary` already covers; no modelled ordering and no `run.sh` expectation changes. - Not modelled: control-request timeouts (see above); the inbound half (`handle_control_request`, the #119 process-exit path, cancellation during a response write, which the transport answers by poisoning itself so no partial frame is ever followed by another); `Metadata.write`'s own temp file; `TranscriptMirrorBatcher`. - **The Ruby is not verified.** A model can drift from the code it describes. When you change one of the orderings the tables cite, update the model and `run.sh`'s expectations in the same change. Each counterexample trace can also be turned into an RSpec regression by stubbing `CLIInstaller::Http` / `Release` / `Metadata`, or with a scripted transport for `Query`, so that the step TLC found is exercised against the real code. diff --git a/lib/claude_agent_sdk/cli_installer.rb b/lib/claude_agent_sdk/cli_installer.rb index ab9a9423..c28bb6ce 100644 --- a/lib/claude_agent_sdk/cli_installer.rb +++ b/lib/claude_agent_sdk/cli_installer.rb @@ -60,6 +60,11 @@ module CLIInstaller # rubocop:disable Metrics/ModuleLength -- Http/Platform/Rele VERSION_FILE = 'VERSION' # @api private LOCK_FILE = '.install.lock' + # How often a waiting installer retries the install lock. See + # .with_install_lock for why it polls instead of blocking in flock. + # + # @api private + LOCK_POLL_SECONDS = 0.05 # Relative to .root (Dir.pwd when unset), resolved at CALL time by # .default_dir — an absolute constant would freeze the working directory # as of require time, which is wrong for anything that chdirs (Rake @@ -168,6 +173,11 @@ def fetch_text(url, limit:) # +max_bytes+ (the manifest's declared size, when it has one) aborts a # response that runs long instead of filling the disk before the # checksum gets a chance to reject it. + # + # The file is fsynced before it is closed: the caller renames it into + # place, and a rename only orders metadata. Without the sync, a power + # loss shortly after an install could leave the published name + # pointing at an empty or partial file. def download_to(url, path, max_bytes: nil) with_response(url) do |response| written = 0 @@ -180,6 +190,7 @@ def download_to(url, path, max_bytes: nil) file.write(chunk) end + file.fsync end end path @@ -195,9 +206,9 @@ def with_response(url, redirects_left = MAX_REDIRECTS, &block) uri = url.is_a?(URI::Generic) ? url : URI(url.to_s) raise CLIInstallError, "Refusing to fetch non-HTTPS URL: #{uri}" unless uri.is_a?(URI::HTTPS) - Net::HTTP.start(uri.host, uri.port, use_ssl: true, - open_timeout: OPEN_TIMEOUT_SECONDS, - read_timeout: READ_TIMEOUT_SECONDS) do |http| + Net::HTTP.start(uri.host, uri.port, *proxy_args(uri), use_ssl: true, + open_timeout: OPEN_TIMEOUT_SECONDS, + read_timeout: READ_TIMEOUT_SECONDS) do |http| http.request(Net::HTTP::Get.new(uri)) do |response| # Branch on the status BEFORE touching the body: a redirect or an # error page must never be streamed into the target file. @@ -213,6 +224,35 @@ def with_response(url, redirects_left = MAX_REDIRECTS, &block) raise CLIInstallError, "Failed to fetch #{url}: #{e.class}: #{e.message}" end + # The proxy arguments for Net::HTTP.start: address, port, user, + # password — or none. + # + # Left to itself, Net::HTTP looks its proxy up as if for an http:// + # URL: it reads http_proxy even though this connection is TLS. An + # environment that exports only HTTPS_PROXY (what curl, RubyGems and + # the CLI itself read for an https URL) was therefore bypassed. + # URI#find_proxy on the https URL reads https_proxy / HTTPS_PROXY and + # applies no_proxy / NO_PROXY; the proxy it names is passed + # explicitly, its credentials percent-decoded the way Net::HTTP + # decodes the ones it finds itself. + # + # No arguments means "as before": Net::HTTP's own lookup (http_proxy) + # stays in charge. That is the answer when the variable is unset or + # no_proxy excludes the host, and also when its value is nothing + # Net::HTTP can use as an HTTP proxy — no scheme, socks5://, https://, + # not a URL at all. Such a value was ignored before and still is, + # rather than turning a download that works directly into a failure. + # ALL_PROXY is not consulted. + def proxy_args(uri) + proxy = uri.find_proxy + return [] unless proxy.instance_of?(URI::HTTP) && !proxy.hostname.to_s.empty? + + credentials = [proxy.user, proxy.password].map { |part| part && URI.decode_www_form_component(part) } + [proxy.hostname, proxy.port, *credentials] + rescue URI::InvalidURIError + [] + end + def follow_redirect(uri, response, redirects_left, &) raise CLIInstallError, "Too many redirects while fetching #{uri}" if redirects_left <= 0 @@ -322,11 +362,14 @@ def read(dir) # Atomic: an unpredictable temp name opened O_EXCL, then renamed over # the old file. Without this a reader could observe a half-written # VERSION, or (worse) the previous version paired with a new binary. + # Fsynced before the rename, so that after a power loss VERSION is + # the old file or the new one, not an empty one. def write(dir, version, checksum, platform) tmp = File.join(dir, "#{VERSION_FILE}.#{SecureRandom.hex(8)}.tmp") begin File.open(tmp, File::WRONLY | File::CREAT | File::EXCL, 0o644) do |file| file.write("#{version}\n#{checksum}\n#{platform}\n") + file.fsync end File.rename(tmp, File.join(dir, VERSION_FILE)) ensure @@ -376,7 +419,7 @@ def default_dir # +version+ is 'stable', 'latest', or a concrete version like '2.1.220'. # # Idempotent and safe to run concurrently: an exclusive lock on - # dir/.install.lock covers the whole check-download-place-record + # dir/.install.lock covers the whole check-download-record-place # sequence, so parallel boots (Docker layers, `foreman start`, CI matrix # jobs sharing a cache) never race each other into a partially written # binary — the loser of the race observes a finished install. @@ -386,6 +429,12 @@ def default_dir # touches the network: repeat boots must work offline (with a pinned # version — a dist-tag has to be re-resolved to be resolved at all). # + # That shortcut also holds in a directory this process cannot write (an + # image built as root and run as another user, a read-only root + # filesystem): a concrete version that is already installed and intact + # is returned. Anything that would need a write — a dist-tag, another + # version, a damaged binary — raises there. + # # An upgrade never destroys a working install: see #publish. def install(version: 'stable', dir: nil) dir = File.expand_path(dir || default_dir) @@ -396,7 +445,19 @@ def install(version: 'stable', dir: nil) requested = Release.validate_version(version) binary = File.join(dir, BINARY_NAME) FileUtils.mkdir_p(dir) - with_install_lock(dir) do + begin + lock = open_lock_file(dir) + rescue Errno::EACCES, Errno::EROFS, Errno::EPERM + # The lock file cannot be opened for writing, so nothing can be + # installed here — and nothing has to be when the requested version + # already is (see .installed_without_lock?). This rescue covers + # OPENING the lock file and nothing else: a permission error raised + # inside the critical section must keep failing the install. + raise unless installed_without_lock?(dir, requested) + + return binary + end + with_install_lock(lock) do sweep_stale_temp_files(dir) resolved = Release.resolve_version(requested) platform = Platform.detect @@ -410,8 +471,11 @@ def install(version: 'stable', dir: nil) rescue SystemCallError, IOError => e # Filesystem failures (EACCES on the install dir, ENOSPC mid-download, # a read-only mount) reach callers as CLIInstallError like every other - # install failure; `cause` keeps the original for debugging. - raise CLIInstallError, "Failed to install the Claude Code CLI into #{dir}: #{e.class}: #{e.message}" + # install failure; `cause` keeps the original for debugging. +dir+ is + # still nil when resolving the default directory is what failed (the + # working directory was deleted): name it by its relative path then. + raise CLIInstallError, + "Failed to install the Claude Code CLI into #{dir || DEFAULT_DIR}: #{e.class}: #{e.message}" end # Install PINNED_CLI_VERSION — the version this gem release was tested @@ -441,20 +505,50 @@ def installed_path(dir: nil) private # Cross-process mutual exclusion for the whole install. flock is - # advisory and per open file description, so concurrent threads in one - # process contend here exactly like separate processes do. - def with_install_lock(dir) + # advisory and per open file description, so concurrent threads — and + # concurrent fibers — in one process contend here exactly like separate + # processes do. + # + # The lock is polled (LOCK_NB + sleep), never taken with a blocking + # LOCK_EX. File#flock has no Fiber-scheduler hook, so a blocking call + # parks the whole reactor THREAD; when the holder is another fiber of + # that reactor, waiting inside the critical section for the network, it + # is never resumed and neither install returns. Kernel#sleep yields to + # a scheduler and is an ordinary sleep on a plain thread. Mutual + # exclusion is the same either way; like flock itself, the order in + # which waiters get the lock is unspecified. + # + # +lock+ is the open lock file (see .open_lock_file); it is closed here. + def with_install_lock(lock) + sleep(LOCK_POLL_SECONDS) until lock.flock(File::LOCK_EX | File::LOCK_NB) + begin + yield + ensure + lock.flock(File::LOCK_UN) + end + ensure + lock.close + end + + # Opened read-write and created when missing. Kept apart from + # .with_install_lock so that .install can tell a lock file that cannot + # be opened from a failure inside the critical section. + def open_lock_file(dir) flags = File::RDWR | File::CREAT # Never follow a symlink planted at the lock path. flags |= File::NOFOLLOW if defined?(File::NOFOLLOW) - File.open(File.join(dir, LOCK_FILE), flags, 0o644) do |lock| - lock.flock(File::LOCK_EX) - begin - yield - ensure - lock.flock(File::LOCK_UN) - end - end + File.open(File.join(dir, LOCK_FILE), flags, 0o644) + end + + # Whether .install can answer +requested+ without the install lock: it + # has to be a concrete version — a dist-tag must be resolved and may + # have to be published, which needs the lock — whose binary is in place + # and re-hashes to the checksum recorded for it. This is the lock-free + # read .installed_path already documents (#publish only ever renames a + # complete, verified binary into place) plus the re-hash, and it writes + # nothing. + def installed_without_lock?(dir, requested) + !DIST_TAGS.include?(requested) && installed?(dir, requested, Platform.detect) end # Remove temp files abandoned by an earlier install that died before its @@ -503,17 +597,38 @@ def installed?(dir, version, platform) # (The reverse order — rename then record — briefly published a binary # nothing vouched for, and a metadata failure then had to delete the # freshly renamed file, taking the previous working install with it.) + # + # Across a power loss, a rename only orders metadata. So the downloaded + # bytes (Http.download_to), the binary's executable mode (after its + # chmod, #fetch_verified) and the VERSION bytes (Metadata.write) are + # fsynced before their renames: each name then holds a complete file, + # the old one or the new one, never an empty, partial or + # non-executable one. The + # directory is synced after the rename so that an install that returned + # is still there afterwards; that sync is best-effort and cannot fail + # the install, which keeps the rename the last step that can. def publish(dir, binary, version, platform, entry) tmp = "#{binary}.download.#{SecureRandom.hex(8)}" begin fetch_verified(version, platform, entry, tmp) Metadata.write(dir, version, entry[:checksum], platform) File.rename(tmp, binary) + sync_directory(dir) ensure FileUtils.rm_f(tmp) end end + # Makes the renames in +dir+ durable (VERSION's and the binary's). + # Best-effort by necessity: it runs after the rename, where nothing may + # fail the install, and not every platform or filesystem can fsync a + # directory — some refuse to open one, others answer EINVAL or EBADF. + def sync_directory(dir) + File.open(dir, File::RDONLY, &:fsync) + rescue SystemCallError, IOError, NotImplementedError + nil + end + # Download to an unpredictable sibling temp name (same filesystem, so the # rename is atomic; O_EXCL, so a pre-planted path or symlink cannot be # written through), bounded by the manifest's declared size, then verify @@ -526,6 +641,12 @@ def fetch_verified(version, platform, entry, tmp) raise CLIInstallError, "Checksum mismatch for #{url}: expected #{expected}, got #{actual}" if actual != expected File.chmod(0o755, tmp) + # The mode is metadata of the file itself: the data fsync in + # Http.download_to came before it, and the directory sync after the + # rename does not cover it. Without this sync a power loss could bring + # the published binary back without its executable bit, and + # installed_path would skip it. + File.open(tmp, File::RDONLY, &:fsync) end end end diff --git a/lib/claude_agent_sdk/sdk_mcp_server.rb b/lib/claude_agent_sdk/sdk_mcp_server.rb index cdb099e3..cac87fb9 100644 --- a/lib/claude_agent_sdk/sdk_mcp_server.rb +++ b/lib/claude_agent_sdk/sdk_mcp_server.rb @@ -36,6 +36,9 @@ def self.deep_normalize_schema(obj) # mangled into nonsense parameter lists ("additionalProperties" as a # required string param). A $ref-only schema without type: 'object' remains # indistinguishable from a params hash — declare the type alongside $ref. + # For the same reason { type: :object } is this prebuilt accept-any-object + # schema even though :object is also a shorthand type: a simple schema with + # an object parameter literally named type has to spell it { type: Hash }. # @api private def self.prebuilt_json_schema?(schema) return false unless schema.is_a?(Hash) @@ -67,13 +70,16 @@ def self.normalize_tool_schema(schema) # @api private def self.ruby_type_to_json_schema(type) # Class#=== matches instances, not the class object used in { id: Integer }. - type = { String => :string, Integer => :integer, Float => :float, - TrueClass => :boolean, FalseClass => :boolean }.fetch(type, type) + type = { String => :string, Integer => :integer, Float => :float, TrueClass => :boolean, + FalseClass => :boolean, Array => :array, Hash => :object }.fetch(type, type) case type when :string, String then { type: 'string' } when :integer, Integer then { type: 'integer' } when :float, Float, :number then { type: 'number' } when :boolean, TrueClass, FalseClass then { type: 'boolean' } + # The class or the Symbol only. An Array or Hash VALUE ([String], a nested + # schema fragment) is not a shorthand and falls through like before. + when :array, :object then { type: type.to_s } else { type: 'string' } # rubocop:disable Lint/DuplicateBranch -- default fallback; the :string arm stays explicit end end @@ -301,9 +307,19 @@ def call_tool(name, arguments) ClaudeAgentSDK.normalize_tool_result(tool.handler.call(arguments)) end - # Guard before flexible_fetch: it raises on non-Hash inputs. - content = result.is_a?(Hash) ? ClaudeAgentSDK.flexible_fetch(result, 'content', 'content') : nil - return error_tool_result("Tool '#{name}' must return a hash with :content key") unless content + # Guard before flexible_fetch: it raises on non-Hash inputs. This first + # diagnostic is about the KEY: a present false or nil is a wrong value + # and gets the Array diagnostic below, like any other non-Array. + unless result.is_a?(Hash) && (result.key?(:content) || result.key?('content')) + return error_tool_result("Tool '#{name}' must return a hash with :content key") + end + + content = ClaudeAgentSDK.flexible_fetch(result, 'content', 'content') + # A String or a single block here is not a result an MCP client accepts. + unless content.is_a?(Array) + return error_tool_result("Tool '#{name}' must return :content as an Array of content blocks " \ + "(got #{content.class})") + end result rescue StandardError => e @@ -412,7 +428,7 @@ def normalize_tools_call_errors(message, response) end # Create dynamic Tool classes from tool definitions - def create_tool_classes(tools) # rubocop:disable Metrics/AbcSize, Metrics/MethodLength -- builds each dynamic MCP::Tool subclass inline + def create_tool_classes(tools) # rubocop:disable Metrics/AbcSize, Metrics/CyclomaticComplexity, Metrics/MethodLength, Metrics/PerceivedComplexity -- builds each dynamic MCP::Tool subclass inline # Captured so the dynamic class can resolve the effective scheduling # mode at call time — same pattern as prompt classes. sdk_server = self @@ -444,16 +460,22 @@ def description_value end def input_schema_value - # Full-schema construction: the gem JSON-round-trips and - # validates against the draft4 metaschema. additionalProperties/ - # enum/description survive. Empty required arrays are stripped — - # draft4's metaschema mandates non-empty required (Python's - # modern jsonschema accepts []). Schemas the draft4 metaschema - # rejects (numeric exclusiveMinimum, $ref/$defs — valid modern - # JSON Schema that Python accepts) fall back to a permissive - # schema with a one-time warning: the tool keeps working with - # argument validation disabled instead of being permanently - # uncallable while tools/list advertises it as healthy. + # Full-schema construction: the gem JSON-round-trips the schema + # and validates it against the JSON Schema 2020-12 metaschema + # (every mcp version this gem supports; older ones used draft4). + # additionalProperties/enum/description survive, and so do a + # numeric exclusiveMinimum and same-document $ref/$defs. Empty + # required arrays are stripped — a holdover from draft4, whose + # metaschema mandated a non-empty required. + # + # A schema the gem refuses (ArgumentError: a draft4-style + # boolean exclusiveMinimum, an unknown type, a $ref that leaves + # the document, an invalid pattern, ...) falls back to a + # permissive schema with a warning, once per tool: the tool + # keeps working with argument validation disabled instead of + # being permanently uncallable while tools/list advertises it + # as healthy. (The warning's "not draft4-compatible" wording + # dates from the draft4 days.) @input_schema_value ||= begin schema = ClaudeAgentSDK.normalize_tool_schema(@tool_def.input_schema) schema = schema.except(:required) if schema[:required].is_a?(Array) && schema[:required].empty? @@ -493,11 +515,21 @@ def call(server_context: nil, **args) # rubocop:disable Lint/UnusedMethodArgumen # Guard BEFORE flexible_fetch: on a non-Hash it raises # TypeError/NoMethodError, surfacing garbage instead of the - # friendly message. - raise "Tool '#{@tool_def.name}' must return a hash with :content key" unless result.is_a?(Hash) + # friendly message. This first diagnostic is about the KEY: a + # present false or nil is a wrong value and gets the Array + # diagnostic below, like any other non-Array. + unless result.is_a?(Hash) && (result.key?(:content) || result.key?('content')) + raise "Tool '#{@tool_def.name}' must return a hash with :content key" + end content = ClaudeAgentSDK.flexible_fetch(result, 'content', 'content') - raise "Tool '#{@tool_def.name}' must return a hash with :content key" if content.nil? + # A String or a single block Hash would go out as it is, and the + # CLI rejects that frame against its schema: the model is told + # the SERVER returned a malformed result and never sees the text. + unless content.is_a?(Array) + raise "Tool '#{@tool_def.name}' must return :content as an Array of content blocks " \ + "(got #{content.class})" + end is_error = ClaudeAgentSDK.flexible_fetch(result, 'isError', 'is_error') structured_content = ClaudeAgentSDK.flexible_fetch(result, 'structuredContent', 'structured_content') @@ -606,7 +638,17 @@ def convert_input_schema(schema) # # @param name [String] Unique identifier for the tool # @param description [String] Human-readable description - # @param input_schema [Hash] Schema defining input parameters + # @param input_schema [Hash] Schema defining input parameters: a full JSON + # Schema (+{ type: 'object', properties: ... }+), or the shorthand + # +{ name: type }+, in which every parameter is required and each type is + # +String+ / +:string+, +Integer+ / +:integer+, +Float+ / +:float+ / + # +:number+, +TrueClass+ / +FalseClass+ / +:boolean+, +Array+ / +:array+ + # or +Hash+ / +:object+ + # @param annotations [Hash, nil] MCP tool annotations (+title+, + # +readOnlyHint+, ...). +maxResultSizeChars+ is also forwarded as + # +_meta['anthropic/maxResultSizeChars']+, the form the CLI reads + # @param meta [Hash, nil] The tool's +_meta+. Merged with the size hint + # derived from +annotations+; a key given here wins # @param handler [Proc] Block that implements the tool logic. It returns a # String, sent to Claude as a single text block, or a Hash with a # +:content+ Array of MCP content blocks plus optional +:is_error+ / @@ -641,11 +683,16 @@ def convert_input_schema(schema) def self.create_tool(name, description, input_schema, annotations: nil, meta: nil, &handler) raise ArgumentError, 'Block required for tool handler' unless handler - # Auto-populate _meta with maxResultSizeChars from annotations if present + # Auto-populate _meta with maxResultSizeChars from annotations if present. + # An explicit meta: is merged with that hint rather than replacing it (an + # unrelated key used to drop it); a size key the caller sets itself wins, + # in either spelling — adding the String key beside a Symbol one would + # put the same JSON key in the frame twice. + size_key = 'anthropic/maxResultSizeChars' resolved_meta = meta - if resolved_meta.nil? && annotations + if annotations.is_a?(Hash) && (meta.nil? || (meta.is_a?(Hash) && meta.keys.none? { |key| key.to_s == size_key })) max_chars = annotations[:maxResultSizeChars] || annotations['maxResultSizeChars'] - resolved_meta = { 'anthropic/maxResultSizeChars' => max_chars } if max_chars + resolved_meta = { size_key => max_chars }.merge(meta || {}) if max_chars end # tools/call arrives from the CLI with the name as a JSON String; the mcp diff --git a/plugins/claude-agent-ruby/skills/claude-agent-ruby/SKILL.md b/plugins/claude-agent-ruby/skills/claude-agent-ruby/SKILL.md index 79aa1c70..2555e078 100644 --- a/plugins/claude-agent-ruby/skills/claude-agent-ruby/SKILL.md +++ b/plugins/claude-agent-ruby/skills/claude-agent-ruby/SKILL.md @@ -68,7 +68,7 @@ options = ClaudeAgentSDK::ClaudeAgentOptions.new( - Include `mcp____` in `allowed_tools` - Use `annotations:` on `create_tool` for MCP tool annotations - Use `meta:` on `create_tool` for `_meta` field forwarding (e.g., `{ 'anthropic/maxResultSizeChars' => 100000 }` to prevent truncation of large results) -- If `annotations[:maxResultSizeChars]` is set, `_meta` is auto-populated +- If `annotations[:maxResultSizeChars]` is set, `_meta` is auto-populated with it; an explicit `meta:` is merged with that hint, and a key set in `meta:` wins - Both symbol-keyed and string-keyed `input_schema` hashes are accepted ## Session Management diff --git a/skills/SKILL.md b/skills/SKILL.md index 79aa1c70..2555e078 100644 --- a/skills/SKILL.md +++ b/skills/SKILL.md @@ -68,7 +68,7 @@ options = ClaudeAgentSDK::ClaudeAgentOptions.new( - Include `mcp____` in `allowed_tools` - Use `annotations:` on `create_tool` for MCP tool annotations - Use `meta:` on `create_tool` for `_meta` field forwarding (e.g., `{ 'anthropic/maxResultSizeChars' => 100000 }` to prevent truncation of large results) -- If `annotations[:maxResultSizeChars]` is set, `_meta` is auto-populated +- If `annotations[:maxResultSizeChars]` is set, `_meta` is auto-populated with it; an explicit `meta:` is merged with that hint, and a key set in `meta:` wins - Both symbol-keyed and string-keyed `input_schema` hashes are accepted ## Session Management diff --git a/spec/unit/cli_installer_durability_spec.rb b/spec/unit/cli_installer_durability_spec.rb new file mode 100644 index 00000000..cc3d486c --- /dev/null +++ b/spec/unit/cli_installer_durability_spec.rb @@ -0,0 +1,244 @@ +# frozen_string_literal: true + +require 'spec_helper' +require 'digest' +require 'json' +require 'tmpdir' + +# Durability of CLIInstaller's publish step. rename() is atomic for concurrent +# readers and across a process crash, but across a power loss it only orders +# metadata: unless the bytes were fsynced first, the published name can come +# back pointing at an empty or partial file — a zero-length executable +# `claude` beside a VERSION that names it, which lock-free discovery then +# hands out. Nothing was fsynced before. The binary's executable mode is set +# after its bytes were synced, so it is synced again after the chmod. +# +# A power cut cannot be staged in a spec, so these examples spy on fsync and +# rename and check that the syncs happen, and where in the sequence. +RSpec.describe ClaudeAgentSDK::CLIInstaller, 'durability of the publish step' do + let(:base) { 'https://downloads.claude.ai/claude-code-releases' } + let(:http) { ClaudeAgentSDK::CLIInstaller::Http } + let(:metadata) { ClaudeAgentSDK::CLIInstaller::Metadata } + let(:dir) { @dir } + let(:binary_path) { File.join(dir, 'claude') } + # [:fsync, name, size at that moment], [:fsync_failed, name], + # [:rename, from, to], [:rename_failed, from, to] and, where an example + # spies on it, [:chmod, name, octal mode], in order; the random part of a + # temp name is replaced by . + let(:events) { [] } + + around do |example| + Dir.mktmpdir('cli-installer-durability') do |tmp| + @dir = File.join(tmp, 'vendor', 'claude') + example.run + end + end + + def label(path) + return 'the install directory' if File.expand_path(path) == dir + + File.basename(path).sub(/[0-9a-f]{16}/, '') + end + + # Records every fsync and every rename, then lets the real one run. + # +failing+ maps a label to the error raised instead: by the fsync of that + # file, or by the rename onto that name. + def spy_on_syncs_and_renames(failing: {}) + spy_on_fsync(failing) + spy_on_rename(failing) + end + + def spy_on_rename(failing) + log = events + labeller = method(:label) + allow(File).to receive(:rename).and_wrap_original do |original, from, to| + names = [labeller.call(from), labeller.call(to)] + if failing.key?(names.last) + log << [:rename_failed, *names] + raise failing[names.last] + end + + original.call(from, to).tap { log << [:rename, *names] } + end + end + + def spy_on_chmod + log = events + labeller = method(:label) + allow(File).to receive(:chmod).and_wrap_original do |original, mode, *paths| + original.call(mode, *paths).tap { paths.each { |path| log << [:chmod, labeller.call(path), mode.to_s(8)] } } + end + end + + def spy_on_fsync(failing) + log = events + labeller = method(:label) + allow_any_instance_of(File).to receive(:fsync).and_wrap_original do |original, *args| + path = original.receiver.path + name = labeller.call(path) + if failing.key?(name) + log << [:fsync_failed, name] + raise failing[name] + end + + original.call(*args).tap { log << [:fsync, name, File.directory?(path) ? nil : File.size(path)] } + end + end + + # The manifest GET is stubbed at Http.fetch_text; the binary comes through + # the real Http.download_to (only the response is canned), so the file is + # written and synced by the code under test. + def stub_release(version:, body:) + allow(ClaudeAgentSDK::CLIInstaller::Platform).to receive(:detect).and_return('linux-x64') + allow(http).to receive(:fetch_text).with("#{base}/#{version}/manifest.json", any_args).and_return( + JSON.generate('platforms' => { 'linux-x64' => { 'checksum' => Digest::SHA256.hexdigest(body), + 'size' => body.bytesize } }) + ) + response = Object.new + response.define_singleton_method(:read_body) { |&chunk| body.scan(/.{1,7}/m).each(&chunk) } + allow(http).to receive(:with_response) { |_url, &handler| handler.call(response) } + end + + it 'syncs the download (bytes, then mode) and the metadata before their renames, and the directory after the last one' do + body = 'not-really-280MB-of-claude' + stub_release(version: '2.1.220', body: body) + spy_on_syncs_and_renames + spy_on_chmod + + expect(described_class.install(version: '2.1.220', dir: dir)).to eq(binary_path) + + metadata_size = "2.1.220\n#{Digest::SHA256.hexdigest(body)}\nlinux-x64\n".bytesize + expect(events).to eq( + [ + [:fsync, 'claude.download.', body.bytesize], + [:chmod, 'claude.download.', '755'], + [:fsync, 'claude.download.', body.bytesize], + [:fsync, 'VERSION..tmp', metadata_size], + [:rename, 'VERSION..tmp', 'VERSION'], + [:rename, 'claude.download.', 'claude'], + [:fsync, 'the install directory', nil] + ] + ) + end + + it 'syncs nothing on the idempotent shortcut, which writes nothing' do + stub_release(version: '2.1.220', body: 'not-really-280MB-of-claude') + described_class.install(version: '2.1.220', dir: dir) + spy_on_syncs_and_renames + + described_class.install(version: '2.1.220', dir: dir) + + expect(events).to be_empty + end + + # After the rename nothing may fail the install: the binary is published and + # VERSION vouches for it. Platforms and filesystems that cannot fsync a + # directory say so in different ways. + [Errno::EINVAL, Errno::EACCES, Errno::EBADF, Errno::ENOTSUP, IOError, NotImplementedError].each do |error| + it "still succeeds when the directory cannot be synced (#{error})" do + stub_release(version: '2.1.220', body: 'not-really-280MB-of-claude') + spy_on_syncs_and_renames(failing: { 'the install directory' => error }) + + expect(described_class.install(version: '2.1.220', dir: dir)).to eq(binary_path) + expect(File.binread(binary_path)).to eq('not-really-280MB-of-claude') + expect(events.last(2)).to eq([[:rename, 'claude.download.', 'claude'], + [:fsync_failed, 'the install directory']]) + end + end + + it 'still succeeds when the directory cannot even be opened for the sync' do + stub_release(version: '2.1.220', body: 'not-really-280MB-of-claude') + spy_on_syncs_and_renames + allow(File).to receive(:open).and_call_original + allow(File).to receive(:open).with(dir, File::RDONLY).and_raise(Errno::EACCES.new(dir)) + + expect(described_class.install(version: '2.1.220', dir: dir)).to eq(binary_path) + expect(File).to have_received(:open).with(dir, File::RDONLY) + expect(events.last).to eq([:rename, 'claude.download.', 'claude']) + end + + # What a FIRST install that fails can leave behind, as docs/cli-installer.md + # states it. VERSION is recorded before the binary is renamed into place + # (that order is what protects a working install during an upgrade), so a + # failure at that last step leaves a VERSION with nothing beside it. + context 'when a first install fails' do + let(:body) { 'not-really-280MB-of-claude' } + + before { stub_release(version: '2.1.220', body: body) } + + it 'publishes no binary but leaves a recorded VERSION when the last step, the rename, fails' do + spy_on_syncs_and_renames(failing: { 'claude' => Errno::EIO }) + + expect { described_class.install(version: '2.1.220', dir: dir) } + .to raise_error(ClaudeAgentSDK::CLIInstallError, /Errno::EIO/) + + expect(events.last(2)).to eq([[:rename, 'VERSION..tmp', 'VERSION'], + [:rename_failed, 'claude.download.', 'claude']]) + expect(Dir.children(dir)).to contain_exactly('.install.lock', 'VERSION') + expect(metadata.read(dir)) + .to eq(version: '2.1.220', checksum: Digest::SHA256.hexdigest(body), platform: 'linux-x64') + # Lock-free discovery finds no binary there and moves on. + expect(described_class.installed_path(dir: dir)).to be_nil + end + + it 'is redone cleanly by the next install, which does not trust the leftover VERSION' do + spy_on_syncs_and_renames(failing: { 'claude' => Errno::EIO }) + expect { described_class.install(version: '2.1.220', dir: dir) }.to raise_error(ClaudeAgentSDK::CLIInstallError) + events.clear + spy_on_syncs_and_renames + + expect(described_class.install(version: '2.1.220', dir: dir)).to eq(binary_path) + + expect(events.map(&:first)).to eq(%i[fsync fsync fsync rename rename fsync]) + expect(File.binread(binary_path)).to eq(body) + expect(Dir.children(dir)).to contain_exactly('.install.lock', 'VERSION', 'claude') + end + + it 'leaves only the directory and its lock file when an earlier step fails' do + spy_on_syncs_and_renames(failing: { 'claude.download.' => Errno::EIO }) + + expect { described_class.install(version: '2.1.220', dir: dir) } + .to raise_error(ClaudeAgentSDK::CLIInstallError, /Errno::EIO/) + + expect(events).to eq([[:fsync_failed, 'claude.download.']]) + expect(Dir.children(dir)).to contain_exactly('.install.lock') + expect(described_class.installed_path(dir: dir)).to be_nil + end + end + + context 'with a working install in place' do + before do + stub_release(version: '2.1.220', body: 'the installed build') + described_class.install(version: '2.1.220', dir: dir) + stub_release(version: '2.1.226', body: 'a newer build') + end + + def expect_previous_install_intact + expect(File.binread(binary_path)).to eq('the installed build') + expect(metadata.read(dir)).to include(version: '2.1.220') + expect(Dir.children(dir)).to contain_exactly('.install.lock', 'VERSION', 'claude') + end + + it 'fails the upgrade, publishing nothing, when the download cannot be synced' do + spy_on_syncs_and_renames(failing: { 'claude.download.' => Errno::EIO }) + + expect { described_class.install(version: '2.1.226', dir: dir) } + .to raise_error(ClaudeAgentSDK::CLIInstallError, /Errno::EIO/) + + expect(events).to eq([[:fsync_failed, 'claude.download.']]) + expect_previous_install_intact + end + + it 'fails the upgrade, publishing nothing, when the metadata cannot be synced' do + spy_on_syncs_and_renames(failing: { 'VERSION..tmp' => Errno::EIO }) + + expect { described_class.install(version: '2.1.226', dir: dir) } + .to raise_error(ClaudeAgentSDK::CLIInstallError, /Errno::EIO/) + + expect(events).to eq([[:fsync, 'claude.download.', 'a newer build'.bytesize], + [:fsync, 'claude.download.', 'a newer build'.bytesize], + [:fsync_failed, 'VERSION..tmp']]) + expect_previous_install_intact + end + end +end diff --git a/spec/unit/cli_installer_error_message_spec.rb b/spec/unit/cli_installer_error_message_spec.rb new file mode 100644 index 00000000..d3672c74 --- /dev/null +++ b/spec/unit/cli_installer_error_message_spec.rb @@ -0,0 +1,34 @@ +# frozen_string_literal: true + +require 'spec_helper' +require 'tmpdir' + +# The message of the CLIInstallError that wraps a filesystem failure names the +# install directory. When resolving that directory is itself what failed (the +# default one, with the process's working directory deleted under it), the +# message used to read "… into : Errno::ENOENT: … getcwd". +RSpec.describe ClaudeAgentSDK::CLIInstaller, 'the message of a wrapped filesystem failure' do + it 'names the default directory when the working directory no longer exists' do + gone = Dir.mktmpdir('cli-installer-cwd') + + Dir.chdir(gone) do + Dir.rmdir(gone) + + expect { described_class.install(version: '2.1.220') } + .to raise_error(ClaudeAgentSDK::CLIInstallError) { |error| + expect(error.message).to start_with('Failed to install the Claude Code CLI into vendor/claude: Errno::ENOENT') + expect(error.cause).to be_a(Errno::ENOENT) + } + end + end + + it 'names the explicit directory otherwise' do + Dir.mktmpdir('cli-installer-message') do |dir| + blocked = File.join(dir, 'a-file') + File.write(blocked, 'not a directory') + + expect { described_class.install(version: '2.1.220', dir: File.join(blocked, 'claude')) } + .to raise_error(ClaudeAgentSDK::CLIInstallError, /\AFailed to install the Claude Code CLI into #{Regexp.escape(blocked)}/) + end + end +end diff --git a/spec/unit/cli_installer_fiber_lock_spec.rb b/spec/unit/cli_installer_fiber_lock_spec.rb new file mode 100644 index 00000000..f880ca14 --- /dev/null +++ b/spec/unit/cli_installer_fiber_lock_spec.rb @@ -0,0 +1,130 @@ +# frozen_string_literal: true + +require 'spec_helper' +require 'open3' +require 'rbconfig' +require 'tmpdir' + +# CLIInstaller.install called from two fibers of ONE reactor, into one +# directory. The install lock used to be a blocking flock(LOCK_EX), and +# File#flock has no Fiber-scheduler hook: the second fiber blocked the reactor +# THREAD while the first one — the lock holder — was parked in a +# scheduler-aware wait inside the critical section, so the holder was never +# resumed and neither install returned. +# +# Runs in a child process: when this regresses, the thread that calls install +# blocks forever, which in-process would hang the suite instead of failing it. +RSpec.describe ClaudeAgentSDK::CLIInstaller, 'called from two fibers of one reactor' do + # The child's own watchdog fires first and says where the reactor thread is + # stuck; the parent's bound only covers a child that cannot even do that. + # Both are far above the fraction of a second the installs take. + child_watchdog_seconds = 30 + parent_bound_seconds = 120 + + # ARGV: the install directory and the point inside the critical section + # where the lock holder waits for the second fiber ('download' or 'resolve'). + # HTTP is replaced at the Http module level, as in cli_installer_spec.rb. + child_script = <<~RUBY + require 'async' + require 'claude_agent_sdk/cli_installer' + require 'digest' + require 'json' + + dir, parking_point = ARGV + installer = ClaudeAgentSDK::CLIInstaller + body = 'not-really-280MB-of-claude' + manifest = JSON.generate( + 'platforms' => { 'linux-x64' => { 'checksum' => Digest::SHA256.hexdigest(body), 'size' => body.bytesize } } + ) + # Closed by the second fiber on its way to the lock. Until then the lock + # holder is parked on it — a scheduler-aware wait inside the critical + # section, which is what the real socket read is. Once closed, pop returns + # at once, so the second fiber never parks here itself. + gate = Thread::Queue.new + downloads = 0 + + installer::Platform.define_singleton_method(:detect) { 'linux-x64' } + installer::Http.define_singleton_method(:fetch_text) do |url, limit:| + if url.end_with?('/stable') + gate.pop if parking_point == 'resolve' + "2.1.220\\n" + elsif url.end_with?('/2.1.220/manifest.json') + manifest + else + raise "unexpected fetch_text(\#{url.inspect})" + end + end + installer::Http.define_singleton_method(:download_to) do |_url, path, max_bytes: nil| + downloads += 1 + gate.pop if parking_point == 'download' + File.binwrite(path, body) + path + end + + # A regression blocks the reactor thread inside File#flock, which releases + # the GVL — this thread still runs and can report where the main one is. + Thread.new do + sleep #{child_watchdog_seconds} + warn "DEADLOCK: neither install returned; main thread at \#{Thread.main.backtrace.first(2).join(' <- ')}" + exit!(3) + end + + # 'resolve': the pinned version is already installed, and both fibers ask + # for the dist-tag — nothing needs installing, yet the tag is resolved + # inside the lock, so the holder still waits on the network there. + installer.install(version: '2.1.220', dir: dir) if parking_point == 'resolve' + version = parking_point == 'resolve' ? 'stable' : '2.1.220' + + paths = Async do |task| + first = task.async { installer.install(version: version, dir: dir) } + second = task.async do + gate.close + installer.install(version: version, dir: dir) + end + [first.wait, second.wait] + end.wait + + puts "paths=\#{paths.inspect} downloads=\#{downloads}" + RUBY + + around do |example| + # Created (and removed) here rather than in the child, so a child that is + # killed mid-install leaves nothing behind. + Dir.mktmpdir('cli-installer-fibers') do |dir| + @dir = dir + example.run + end + end + + let(:dir) { @dir } + let(:binary_path) { File.join(dir, 'claude') } + + define_method(:run_two_fibers) do |parking_point| + lib_dir = File.expand_path('../../lib', __dir__) + Open3.popen3(RbConfig.ruby, '-I', lib_dir, '-e', child_script, dir, parking_point) do |stdin, stdout, stderr, waiter| + stdin.close + unless waiter.join(parent_bound_seconds) + Process.kill('KILL', waiter.pid) + waiter.join + end + [stdout.read, stderr.read, waiter.value] + end + end + + it 'finishes both installs when the lock holder is parked in its download' do + out, err, status = run_two_fibers('download') + + expect(status.exitstatus).to eq(0), "child: #{status.inspect}\n#{err}" + expect(out).to include("paths=#{[binary_path, binary_path].inspect} downloads=1") + expect(File.binread(binary_path)).to eq('not-really-280MB-of-claude') + expect(Dir.children(dir)).to contain_exactly('.install.lock', 'VERSION', 'claude') + end + + it 'finishes both installs of an installed dist-tag while the holder resolves it' do + out, err, status = run_two_fibers('resolve') + + expect(status.exitstatus).to eq(0), "child: #{status.inspect}\n#{err}" + # The one download is the setup install; neither fiber needed another. + expect(out).to include("paths=#{[binary_path, binary_path].inspect} downloads=1") + end +end diff --git a/spec/unit/cli_installer_http_spec.rb b/spec/unit/cli_installer_http_spec.rb new file mode 100644 index 00000000..407cf2d2 --- /dev/null +++ b/spec/unit/cli_installer_http_spec.rb @@ -0,0 +1,293 @@ +# frozen_string_literal: true + +require 'spec_helper' +require 'net/http' +require 'stringio' +require 'tmpdir' + +# CLIInstaller::Http itself. The other installer specs replace +# Http.fetch_text / Http.download_to wholesale; here the only seam is +# Net::HTTP.start, so everything above the socket is the real code: proxy +# resolution, the HTTPS-only check, redirects, status handling and body +# streaming. Responses are real Net::HTTPResponse objects, parsed from the raw +# bytes a server would send — stdlib only, no HTTP stubbing library. +RSpec.describe ClaudeAgentSDK::CLIInstaller::Http do + proxy_variables = %w[http_proxy HTTP_PROXY https_proxy HTTPS_PROXY all_proxy ALL_PROXY no_proxy NO_PROXY].freeze + + # The proxy settings of whoever runs the suite must not leak into an + # example, and what an example sets must not leak out. + around do |example| + saved = proxy_variables.to_h { |name| [name, ENV.fetch(name, nil)] } + proxy_variables.each { |name| ENV.delete(name) } + example.run + ensure + saved.each { |name, value| value.nil? ? ENV.delete(name) : ENV[name] = value } + end + + # One entry per Net::HTTP.start call, in order: where it connected, the + # explicit proxy arguments (none = Net::HTTP's own environment lookup), the + # session options, and the request sent over it. + let(:connections) { [] } + + def raw_response(status, headers = {}, body = '') + head = { 'Content-Length' => body.bytesize }.merge(headers).map { |name, value| "#{name}: #{value}\r\n" }.join + "HTTP/1.1 #{status}\r\n#{head}\r\n#{body}" + end + + # Answers the n-th connection with the n-th raw response, read the way + # Net::HTTP#transport_request reads one off the socket. + def stub_connections(*raw_responses) + allow(Net::HTTP).to receive(:start) do |address, port, *proxy, **options, &session| + raw = raw_responses.shift or raise "unexpected connection to #{address}" + connection = { address: address, port: port, proxy: proxy, options: options } + connections << connection + session.call(connection_answering(raw, connection)) + end + end + + def connection_answering(raw, connection) + http = Object.new + http.define_singleton_method(:request) do |request, &handler| + connection[:request] = request + socket = Net::BufferedIO.new(StringIO.new(raw)) + response = Net::HTTPResponse.read_new(socket) + response.decode_content = request.decode_content + response.reading_body(socket, request.response_body_permitted?) { handler.call(response) } + end + http + end + + describe 'proxy selection' do + # TEST-NET-1 literals: URI#find_proxy resolves the host before applying + # no_proxy, and a literal resolves without DNS. (Not a loopback address — + # find_proxy never proxies those.) + let(:url) { 'https://192.0.2.1/claude-code-releases/stable' } + let(:version_response) { raw_response('200 OK', { 'Content-Type' => 'text/plain' }, "2.1.285\n") } + + def proxy_used_for(url) + stub_connections(version_response) + expect(described_class.fetch_text(url, limit: 1024)).to eq("2.1.285\n") + connections.last[:proxy] + end + + it 'connects through the proxy HTTPS_PROXY names' do + ENV['HTTPS_PROXY'] = 'http://proxy.corp.example:3128' + + expect(proxy_used_for(url)).to eq(['proxy.corp.example', 3128, nil, nil]) + expect(connections.last).to include(address: '192.0.2.1', port: 443) + end + + it 'reads the lowercase https_proxy too' do + ENV['https_proxy'] = 'http://proxy.corp.example:3128' + + expect(proxy_used_for(url)).to eq(['proxy.corp.example', 3128, nil, nil]) + end + + it 'passes the proxy credentials, percent-decoded' do + ENV['HTTPS_PROXY'] = 'http://alice:p%40ss%3Aword@proxy.corp.example:3128' + + expect(proxy_used_for(url)).to eq(['proxy.corp.example', 3128, 'alice', 'p@ss:word']) + end + + it 'unbrackets an IPv6 proxy address and defaults the port' do + ENV['HTTPS_PROXY'] = 'http://[2001:db8::10]' + + expect(proxy_used_for(url)).to eq(['2001:db8::10', 80, nil, nil]) + end + + it 'passes no proxy when NO_PROXY lists the host' do + ENV['HTTPS_PROXY'] = 'http://proxy.corp.example:3128' + ENV['NO_PROXY'] = 'localhost,192.0.2.1' + + expect(proxy_used_for(url)).to eq([]) + end + + it 'passes no proxy when nothing is set' do + expect(proxy_used_for(url)).to eq([]) + end + + it 'leaves an environment that only sets http_proxy to Net::HTTP, as before' do + # No explicit arguments: Net::HTTP's own lookup reads http_proxy, which + # is how the installer reached a proxy before it read HTTPS_PROXY. + ENV['http_proxy'] = 'http://proxy.corp.example:3128' + + expect(proxy_used_for(url)).to eq([]) + end + + it 'does not consult ALL_PROXY' do + ENV['ALL_PROXY'] = 'http://proxy.corp.example:3128' + + expect(proxy_used_for(url)).to eq([]) + end + + it 'resolves the proxy again for every redirect hop' do + ENV['HTTPS_PROXY'] = 'http://proxy.corp.example:3128' + ENV['NO_PROXY'] = '192.0.2.99' + stub_connections(raw_response('302 Found', { 'Location' => 'https://192.0.2.99/claude' }), + raw_response('200 OK', {}, 'binary')) + + expect(described_class.fetch_text(url, limit: 1024)).to eq('binary') + expect(connections.map { |c| [c[:address], c[:proxy]] }) + .to eq([['192.0.2.1', ['proxy.corp.example', 3128, nil, nil]], ['192.0.2.99', []]]) + end + + # Values that were ignored while the installer did not read HTTPS_PROXY at + # all, and that Net::HTTP could not use as an HTTP proxy anyway. Passing + # them on would turn a download that works directly into a failure. + ['proxy.corp.example:3128', '127.0.0.1:3128', 'socks5://127.0.0.1:1080', + 'https://proxy.corp.example', 'http://proxy with spaces:3128', 'http://:3128'].each do |value| + it "still ignores HTTPS_PROXY=#{value}, which is not an http:// proxy URL" do + ENV['HTTPS_PROXY'] = value + + expect(proxy_used_for(url)).to eq([]) + end + end + end + + # With no proxy variable set, find_proxy answers before resolving anything, + # so these can name the real release host. + let(:release_url) { 'https://downloads.claude.ai/claude-code-releases/2.1.285/darwin-arm64/claude' } + + def redirect_to(location, body = '') + raw_response('302 Found', { 'Location' => location }, body) + end + + describe 'the HTTPS-only rule' do + it 'refuses an http:// URL before opening any connection' do + expect(Net::HTTP).not_to receive(:start) + + expect { described_class.fetch_text('http://downloads.claude.ai/claude-code-releases/stable', limit: 1024) } + .to raise_error(ClaudeAgentSDK::CLIInstallError, + 'Refusing to fetch non-HTTPS URL: http://downloads.claude.ai/claude-code-releases/stable') + end + + it 'refuses a redirect from https to http without following it' do + stub_connections(redirect_to('http://mirror.example/claude'), raw_response('200 OK', {}, 'plaintext')) + + expect { described_class.fetch_text(release_url, limit: 1024) } + .to raise_error(ClaudeAgentSDK::CLIInstallError, 'Refusing to fetch non-HTTPS URL: http://mirror.example/claude') + expect(connections.map { |c| c[:address] }).to eq(['downloads.claude.ai']) + end + + it 'opens every connection with TLS and peer verification, on the first hop and after a redirect' do + # use_ssl: true is what puts TLS on the wire: Net::HTTP would speak TLS + # to the host of an http:// URL too. The scheme check above keeps + # HTTP-shaped URLs out; it is not what encrypts the transfer. + stub_connections(redirect_to('https://cdn.example/claude'), raw_response('200 OK', {}, 'binary')) + + described_class.fetch_text(release_url, limit: 1024) + + expect(connections.map { |c| [c[:address], c[:port]] }).to eq([['downloads.claude.ai', 443], ['cdn.example', 443]]) + expect(connections.map { |c| c[:options] }).to all( + eq(use_ssl: true, open_timeout: described_class::OPEN_TIMEOUT_SECONDS, + read_timeout: described_class::READ_TIMEOUT_SECONDS) + ) + end + end + + describe 'redirects' do + it 'sends a GET for the URL and returns what the block makes of the response' do + stub_connections(raw_response('200 OK', { 'Content-Type' => 'text/plain' }, "2.1.285\n")) + + expect(described_class.fetch_text('https://downloads.claude.ai/claude-code-releases/stable', limit: 1024)) + .to eq("2.1.285\n") + request = connections.first[:request] + expect(request).to be_a(Net::HTTP::Get) + expect(request.path).to eq('/claude-code-releases/stable') + expect(request['host']).to eq('downloads.claude.ai') + end + + it 'follows up to five redirects' do + hops = (1..5).map { |n| redirect_to("https://hop#{n}.example/claude") } + stub_connections(*hops, raw_response('200 OK', {}, 'binary')) + + expect(described_class.fetch_text(release_url, limit: 1024)).to eq('binary') + expect(connections.map { |c| c[:address] }) + .to eq(%w[downloads.claude.ai hop1.example hop2.example hop3.example hop4.example hop5.example]) + end + + it 'raises "Too many redirects" on the sixth, after exactly six requests' do + hops = (1..8).map { |n| redirect_to("https://hop#{n}.example/claude") } + stub_connections(*hops) + + expect { described_class.fetch_text(release_url, limit: 1024) } + .to raise_error(ClaudeAgentSDK::CLIInstallError, + 'Too many redirects while fetching https://hop5.example/claude') + expect(connections.size).to eq(6) + end + + it 'resolves a relative Location against the URL that sent it' do + stub_connections(redirect_to('/mirror/2.1.285/claude'), raw_response('200 OK', {}, 'binary')) + + described_class.fetch_text(release_url, limit: 1024) + + expect(connections.last[:address]).to eq('downloads.claude.ai') + expect(connections.last[:request].path).to eq('/mirror/2.1.285/claude') + end + + it 'raises on a redirect without a Location header' do + stub_connections(raw_response('302 Found')) + + expect { described_class.fetch_text(release_url, limit: 1024) } + .to raise_error(ClaudeAgentSDK::CLIInstallError, "Redirect from #{release_url} is missing a Location header") + end + end + + describe 'failures' do + it 'raises with the status line of a 404' do + stub_connections(raw_response('404 Not Found', { 'Content-Type' => 'application/xml' }, 'NoSuchKey')) + + expect { described_class.fetch_text(release_url, limit: 1024) } + .to raise_error(ClaudeAgentSDK::CLIInstallError, "HTTP 404 Not Found for #{release_url}") + end + + it 'wraps a SocketError in CLIInstallError, keeping it as the cause' do + allow(Net::HTTP).to receive(:start).and_raise( + SocketError, 'getaddrinfo: nodename nor servname provided, or not known' + ) + + expect { described_class.fetch_text(release_url, limit: 1024) } + .to raise_error(ClaudeAgentSDK::CLIInstallError) { |error| + expect(error.message).to eq( + "Failed to fetch #{release_url}: SocketError: getaddrinfo: nodename nor servname provided, or not known" + ) + expect(error.cause).to be_a(SocketError) + } + end + + it 'lets a CLIInstallError from the body handler through unwrapped' do + stub_connections(raw_response('200 OK', {}, 'x' * 2048)) + + expect { described_class.fetch_text(release_url, limit: 1024) } + .to raise_error(ClaudeAgentSDK::CLIInstallError, "Response from #{release_url} exceeds the 1024-byte limit") + end + end + + describe 'what reaches the target file' do + around do |example| + Dir.mktmpdir('cli-installer-http') do |dir| + @target = File.join(dir, 'claude.download.0123456789abcdef') + example.run + end + end + + let(:target) { @target } + + it 'streams only the final 200 body, never a redirect body' do + chunked = "HTTP/1.1 200 OK\r\nTransfer-Encoding: chunked\r\n\r\n" \ + "6\r\nbinary\r\n7\r\n-bytes!\r\n0\r\n\r\n" + stub_connections(redirect_to('https://cdn.example/claude', 'Moved'), chunked) + + expect(described_class.download_to(release_url, target, max_bytes: 13)).to eq(target) + expect(File.binread(target)).to eq('binary-bytes!') + end + + it 'creates no file for an error page' do + stub_connections(raw_response('503 Service Unavailable', {}, 'Try again later')) + + expect { described_class.download_to(release_url, target) } + .to raise_error(ClaudeAgentSDK::CLIInstallError, "HTTP 503 Service Unavailable for #{release_url}") + expect(File.exist?(target)).to be false + end + end +end diff --git a/spec/unit/cli_installer_read_only_spec.rb b/spec/unit/cli_installer_read_only_spec.rb new file mode 100644 index 00000000..cfaba5fe --- /dev/null +++ b/spec/unit/cli_installer_read_only_spec.rb @@ -0,0 +1,183 @@ +# frozen_string_literal: true + +require 'spec_helper' +require 'digest' +require 'fileutils' +require 'json' +require 'tmpdir' + +# The offline, idempotent shortcut of CLIInstaller.install in a directory the +# process cannot write: an image built as root and run as another user, or a +# read-only root filesystem. The shortcut sits inside the install lock, and the +# lock file is opened read-write first — so a boot-time install_pinned raised +# EACCES although the pinned binary was installed and intact. +RSpec.describe ClaudeAgentSDK::CLIInstaller, 'in a read-only install directory' do + let(:base) { 'https://downloads.claude.ai/claude-code-releases' } + let(:binary_body) { 'not-really-280MB-of-claude' } + let(:http) { ClaudeAgentSDK::CLIInstaller::Http } + let(:dir) { @dir } + let(:binary_path) { File.join(dir, 'claude') } + let(:lock_path) { File.join(dir, '.install.lock') } + + around do |example| + Dir.mktmpdir('cli-installer-read-only') do |tmp| + @dir = File.join(tmp, 'vendor', 'claude') + example.run + ensure + # mktmpdir cannot remove a directory it may not write. + FileUtils.chmod(0o755, @dir) if File.directory?(@dir) + end + end + + # The two GET endpoints, as cli_installer_spec.rb stubs them: no HTTP + # library, the real Digest / Metadata / rename path. + def stub_release(version: '2.1.220', platform: 'linux-x64') + allow(ClaudeAgentSDK::CLIInstaller::Platform).to receive(:detect).and_return(platform) + allow(http).to receive(:fetch_text) do |url, **| + case url + when "#{base}/stable" then "#{version}\n" + when "#{base}/#{version}/manifest.json" + entry = { 'checksum' => Digest::SHA256.hexdigest(binary_body), 'size' => binary_body.bytesize } + JSON.generate('platforms' => { platform => entry }) + else raise "unexpected fetch_text(#{url.inspect})" + end + end + allow(http).to receive(:download_to) do |_url, path, **| + File.binwrite(path, binary_body) + path + end + end + + def expect_no_network + expect(http).not_to receive(:fetch_text) + expect(http).not_to receive(:download_to) + end + + def expect_permission_failure(error) + expect(error.message).to include("Failed to install the Claude Code CLI into #{dir}", 'Errno::EACCES') + expect(error.cause).to be_a(Errno::EACCES) + end + + before do + stub_release + described_class.install(version: '2.1.220', dir: dir) + end + + # chmod stands in for the foreign owner, so the process must be one that + # file modes apply to. + context 'when file modes deny writing' do + before { skip 'file modes do not restrict root' if Process.uid.zero? } + + # What the runtime user of an image built by another user sees: everything + # readable and executable, nothing writable. + def make_read_only(lock_file: :present) + lock_file == :present ? File.chmod(0o444, lock_path) : File.delete(lock_path) + File.chmod(0o555, dir) + end + + it 'returns the installed binary for its concrete version, offline and without the lock' do + make_read_only + expect_no_network + + expect(described_class.install(version: '2.1.220', dir: dir)).to eq(binary_path) + end + + it 'returns the installed binary when the directory has no lock file to open' do + # The install directory was copied into the image without its dotfile. + make_read_only(lock_file: :absent) + expect_no_network + + expect(described_class.install(version: '2.1.220', dir: dir)).to eq(binary_path) + expect(Dir.children(dir)).to contain_exactly('VERSION', 'claude') + end + + it 'serves install_pinned the same way' do + stub_const('ClaudeAgentSDK::CLIInstaller::PINNED_CLI_VERSION', '2.1.220') + make_read_only + expect_no_network + + expect(described_class.install_pinned(dir: dir)).to eq(binary_path) + end + + it 'raises for a dist-tag, which has to be resolved and may have to be published' do + make_read_only + expect_no_network + + expect { described_class.install(version: 'stable', dir: dir) } + .to raise_error(ClaudeAgentSDK::CLIInstallError) { |error| expect_permission_failure(error) } + end + + it 'raises when the binary no longer matches its recorded checksum' do + File.binwrite(binary_path, 'truncated-or-swapped') + make_read_only + expect_no_network + + expect { described_class.install(version: '2.1.220', dir: dir) } + .to raise_error(ClaudeAgentSDK::CLIInstallError) { |error| expect_permission_failure(error) } + end + + it 'raises when another version is requested than the one installed' do + make_read_only + expect_no_network + + expect { described_class.install(version: '2.1.226', dir: dir) } + .to raise_error(ClaudeAgentSDK::CLIInstallError) { |error| expect_permission_failure(error) } + end + + it 'raises when the binary was installed for another platform' do + make_read_only + allow(ClaudeAgentSDK::CLIInstaller::Platform).to receive(:detect).and_return('linux-arm64-musl') + expect_no_network + + expect { described_class.install(version: '2.1.220', dir: dir) } + .to raise_error(ClaudeAgentSDK::CLIInstallError) { |error| expect_permission_failure(error) } + end + + it 'raises when nothing is installed' do + FileUtils.rm_f([binary_path, File.join(dir, 'VERSION')]) + make_read_only + expect_no_network + + expect { described_class.install(version: '2.1.220', dir: dir) } + .to raise_error(ClaudeAgentSDK::CLIInstallError) { |error| expect_permission_failure(error) } + end + end + + # Errors chmod cannot produce: a read-only filesystem (EROFS) and a denied + # operation (EPERM: an immutable file, a sandbox profile). Injected where + # the kernel would raise them — at the open of the lock file. + context 'when opening the lock file fails' do + def fail_lock_open(error) + allow(File).to receive(:open).and_call_original + allow(File).to receive(:open).with(lock_path, any_args).and_raise(error) + end + + [Errno::EROFS, Errno::EPERM, Errno::EACCES].each do |errno| + it "returns the installed binary for its concrete version on #{errno}" do + fail_lock_open(errno.new(lock_path)) + expect_no_network + + expect(described_class.install(version: '2.1.220', dir: dir)).to eq(binary_path) + end + + it "still raises for a dist-tag on #{errno}" do + fail_lock_open(errno.new(lock_path)) + expect_no_network + + expect { described_class.install(version: 'stable', dir: dir) } + .to raise_error(ClaudeAgentSDK::CLIInstallError) { |error| expect(error.cause).to be_a(errno) } + end + end + + it 'raises on any other failure to open the lock file, even when the binary is intact' do + # A symlink planted at the lock path (opened NOFOLLOW) is not a + # read-only directory, and neither is running out of descriptors. + [Errno::ELOOP, Errno::EMFILE].each do |errno| + fail_lock_open(errno.new(lock_path)) + + expect { described_class.install(version: '2.1.220', dir: dir) } + .to raise_error(ClaudeAgentSDK::CLIInstallError) { |error| expect(error.cause).to be_a(errno) } + end + end + end +end diff --git a/spec/unit/cli_installer_spec.rb b/spec/unit/cli_installer_spec.rb index 343b4dc2..1f94172f 100644 --- a/spec/unit/cli_installer_spec.rb +++ b/spec/unit/cli_installer_spec.rb @@ -77,9 +77,11 @@ def eventually?(timeout: 5) true end - # True while +thread+ is parked inside File#flock (its innermost frame). - def blocked_in_flock?(thread) - thread.backtrace_locations&.first&.base_label == 'flock' + # True while +thread+ is waiting for the install lock: in with_install_lock's + # poll loop, asleep between attempts or inside the non-blocking flock call. + def waiting_for_install_lock?(thread) + innermost, caller = thread.backtrace_locations&.first(2)&.map(&:base_label) + %w[sleep flock].include?(innermost) && caller == 'with_install_lock' end describe '.default_dir' do @@ -680,11 +682,11 @@ def stub_response(*chunks) contended = nil allow(http).to receive(:download_to) do |_url, path, **| counter_mutex.synchronize { downloads += 1 } - # Hold the lock until the other installer is observably blocked in - # flock (bounded poll) rather than sleeping and hoping it got there. + # Hold the lock until the other installer is observably waiting for + # it (bounded poll) rather than sleeping and hoping it got there. # Without the lock the other thread would reach its own download # instead: the poll times out and downloads ends up 2. - contended = eventually? { threads.any? { |t| !t.equal?(Thread.current) && blocked_in_flock?(t) } } + contended = eventually? { threads.any? { |t| !t.equal?(Thread.current) && waiting_for_install_lock?(t) } } File.binwrite(path, binary_body) path end diff --git a/spec/unit/sdk_mcp_tool_meta_spec.rb b/spec/unit/sdk_mcp_tool_meta_spec.rb new file mode 100644 index 00000000..78f7be77 --- /dev/null +++ b/spec/unit/sdk_mcp_tool_meta_spec.rb @@ -0,0 +1,85 @@ +# frozen_string_literal: true + +require 'spec_helper' +require 'json' + +# create_tool forwards annotations[:maxResultSizeChars] to the CLI as +# _meta['anthropic/maxResultSizeChars'] (MCP clients drop the annotation +# itself). It only did so when no meta: was passed: any explicit meta:, even +# one about something else, replaced the size hint. +RSpec.describe ClaudeAgentSDK, '.create_tool _meta from annotations and meta:' do + size_key = 'anthropic/maxResultSizeChars' + + def tool_with(**options) + described_class.create_tool('export_rows', 'Export rows', {}, **options) { |_args| 'rows' } + end + + def advertised_meta(tool) + described_class::SdkMcpServer.new(name: 'reports', tools: [tool]).list_tools.first[:_meta] + end + + it 'derives the size hint from the annotation when no meta: is given' do + tool = tool_with(annotations: { maxResultSizeChars: 500_000 }) + + expect(tool.meta).to eq(size_key => 500_000) + end + + it 'merges an unrelated meta: with the derived size hint instead of dropping it' do + tool = tool_with(annotations: { maxResultSizeChars: 500_000 }, meta: { 'acme/team' => 'billing' }) + + expect(tool.meta).to eq(size_key => 500_000, 'acme/team' => 'billing') + expect(advertised_meta(tool)).to eq(size_key => 500_000, 'acme/team' => 'billing') + end + + it 'advertises both keys through the mcp gem\'s tools/list too' do + tool = tool_with(annotations: { maxResultSizeChars: 500_000 }, meta: { 'acme/team' => 'billing' }) + server = described_class::SdkMcpServer.new(name: 'reports', tools: [tool]) + + listed = JSON.parse(server.handle_json(JSON.generate(jsonrpc: '2.0', id: 1, method: 'tools/list'))) + + expect(listed.dig('result', 'tools', 0, '_meta')).to eq(size_key => 500_000, 'acme/team' => 'billing') + end + + it 'reads the annotation under a String key as well' do + tool = tool_with(annotations: { 'maxResultSizeChars' => 500_000 }, meta: { 'acme/team' => 'billing' }) + + expect(tool.meta).to eq(size_key => 500_000, 'acme/team' => 'billing') + end + + it 'lets a size key set in meta: win over the annotation' do + tool = tool_with(annotations: { maxResultSizeChars: 500_000 }, meta: { size_key => 100, 'acme/team' => 'billing' }) + + expect(tool.meta).to eq(size_key => 100, 'acme/team' => 'billing') + end + + it 'lets a Symbol-keyed size key in meta: win, without a second spelling of the key' do + tool = tool_with(annotations: { maxResultSizeChars: 500_000 }, meta: { size_key.to_sym => 100 }) + + expect(tool.meta).to eq(size_key.to_sym => 100) + # Counted in the serialized frame: two spellings of one key are a + # duplicate JSON key, which json 3.x refuses to generate. + expect(JSON.generate(advertised_meta(tool)).scan(size_key).size).to eq(1) + end + + it 'does not modify the Hash passed as meta:' do + meta = { 'acme/team' => 'billing' }.freeze + + tool = tool_with(annotations: { maxResultSizeChars: 500_000 }, meta: meta) + + expect(meta).to eq('acme/team' => 'billing') + expect(tool.meta).not_to equal(meta) + end + + it 'passes meta: through untouched when the annotations carry no size' do + meta = { 'acme/team' => 'billing' } + + expect(tool_with(annotations: { readOnlyHint: true }, meta: meta).meta).to equal(meta) + expect(tool_with(meta: meta).meta).to equal(meta) + end + + it 'leaves _meta out when there is neither a size annotation nor meta:' do + expect(tool_with(annotations: { readOnlyHint: true }).meta).to be_nil + expect(tool_with.meta).to be_nil + expect(described_class::SdkMcpServer.new(name: 'reports', tools: [tool_with]).list_tools.first).not_to have_key(:_meta) + end +end diff --git a/spec/unit/sdk_mcp_tool_result_spec.rb b/spec/unit/sdk_mcp_tool_result_spec.rb new file mode 100644 index 00000000..ee3fde84 --- /dev/null +++ b/spec/unit/sdk_mcp_tool_result_spec.rb @@ -0,0 +1,105 @@ +# frozen_string_literal: true + +require 'spec_helper' +require 'json' + +# A handler may return a String, or a Hash whose :content is an Array of +# content blocks. A Hash whose :content is itself a String (an easy slip next +# to the String form) or a single block Hash used to be forwarded as it was: +# the CLI rejected the frame against its schema and told the model that the +# SERVER "returned a malformed result", so the text never reached it. +RSpec.describe ClaudeAgentSDK::SdkMcpServer, 'a tool result whose :content is not an Array' do + def server_returning(result) + tool = ClaudeAgentSDK.create_tool('order_status', 'Look up an order', { id: :string }) { |_args| result } + described_class.new(name: 'shop', tools: [tool]) + end + + # tools/call the way a session routes it: the frame from the CLI's + # mcp_message control request, parsed with symbol keys, through the mcp gem. + def call_through_session(server) + wire = JSON.generate(jsonrpc: '2.0', id: 3, method: 'tools/call', + params: { name: 'order_status', arguments: { id: 'A-7' } }) + server.handle_message(JSON.parse(wire, symbolize_names: true))[:result] + end + + def expect_content_error(result, got) + expect(result[:isError]).to be true + expect(result[:content]).to eq( + [{ type: 'text', + text: "Tool 'order_status' must return :content as an Array of content blocks (got #{got})" }] + ) + end + + [ + ['a String', { content: 'Order A-7 has shipped' }, 'String'], + ['a single block Hash', { content: { type: 'text', text: 'Order A-7 has shipped' } }, 'Hash'], + ['a String under a String key', { 'content' => 'Order A-7 has shipped' }, 'String'], + ['an Integer', { content: 7 }, 'Integer'], + # Present, but falsy: the key is there, so this is a wrong value, not a + # missing key. + ['false', { content: false }, 'FalseClass'], + ['a present nil', { content: nil }, 'NilClass'], + ['a present nil under a String key', { 'content' => nil }, 'NilClass'] + ].each do |label, handler_result, got| + it "answers a session's tools/call with an in-band error when :content is #{label}" do + expect_content_error(call_through_session(server_returning(handler_result)), got) + end + + it "returns the same error from #call_tool when :content is #{label}" do + expect_content_error(server_returning(handler_result).call_tool('order_status', { id: 'A-7' }), got) + end + end + + it 'keeps the handler\'s own is_error flag out of it: the result is an error either way' do + result = call_through_session(server_returning({ content: 'fine', is_error: false })) + + expect_content_error(result, 'String') + end + + # The other diagnostic is about the KEY, and only about the key: a result + # that is not a Hash, or a Hash with no :content under either spelling. + [ + ['nil', nil], + ['an Array of blocks without the Hash around it', [{ type: 'text', text: 'Order A-7 has shipped' }]], + ['a Hash without the key', { text: 'Order A-7 has shipped' }], + ['a Hash whose only key is :contents', { contents: [{ type: 'text', text: 'Order A-7 has shipped' }] }] + ].each do |label, handler_result| + it "says the :content key is missing, on both paths, when the handler returns #{label}" do + server = server_returning(handler_result) + missing = [{ type: 'text', text: "Tool 'order_status' must return a hash with :content key" }] + + session = call_through_session(server) + expect(session[:isError]).to be true + expect(session[:content]).to eq(missing) + + expect(server.call_tool('order_status', { id: 'A-7' })).to eq(content: missing, isError: true) + end + end + + it 'reads the Array under either spelling when the other one is nil' do + server = server_returning({ content: nil, 'content' => [{ type: 'text', text: 'shipped' }] }) + + expect(call_through_session(server)).to include(isError: false, content: [{ type: 'text', text: 'shipped' }]) + expect(server.call_tool('order_status', { id: 'A-7' })).not_to have_key(:isError) + end + + # The legal shapes are untouched. + [ + ['a String', 'Order A-7 has shipped', [{ type: 'text', text: 'Order A-7 has shipped' }]], + ['an Array of blocks', { content: [{ type: 'text', text: 'shipped' }, { type: 'text', text: 'on time' }] }, + [{ type: 'text', text: 'shipped' }, { type: 'text', text: 'on time' }]], + ['an empty Array', { content: [] }, []] + ].each do |label, handler_result, content| + it "still passes #{label} through on both paths" do + server = server_returning(handler_result) + + session = call_through_session(server) + expect(session[:isError]).to be false + expect(session[:content]).to eq(content) + + direct = server.call_tool('order_status', { id: 'A-7' }) + expect(direct[:content]).to eq(content) + expect(direct).not_to have_key(:isError) + end + end +end diff --git a/spec/unit/sdk_mcp_tool_schema_spec.rb b/spec/unit/sdk_mcp_tool_schema_spec.rb new file mode 100644 index 00000000..ce0cb49e --- /dev/null +++ b/spec/unit/sdk_mcp_tool_schema_spec.rb @@ -0,0 +1,112 @@ +# frozen_string_literal: true + +require 'spec_helper' +require 'json' + +# Shorthand tool schemas ({ name: type }) for collection parameters. +# `{ tags: Array }` used to be advertised as a string: the model then sent +# "fragile,gift" where the handler expected an Array, and a model that did +# send an array had its call rejected by argument validation. +# +# A session answers tools/list from SdkMcpServer#list_tools and routes +# tools/call through #handle_message (the mcp gem, which validates the +# arguments), so the two are checked against each other here. +RSpec.describe ClaudeAgentSDK::SdkMcpServer, 'shorthand schemas for Array and Hash parameters' do + def server_with(schema, received = []) + tool = ClaudeAgentSDK.create_tool('tag_order', 'Tag an order', schema) do |args| + received << args + "tagged with #{args.inspect}" + end + described_class.new(name: 'shop', tools: [tool]) + end + + # The frame the CLI embeds in an mcp_message control request, as the + # transport hands it over: parsed with symbol keys. + def call_tool(server, arguments) + wire = JSON.generate(jsonrpc: '2.0', id: 7, method: 'tools/call', + params: { name: 'tag_order', arguments: arguments }) + server.handle_message(JSON.parse(wire, symbolize_names: true)) + end + + def advertised_type(server, param) + server.list_tools.first.dig(:inputSchema, :properties, param, :type) + end + + [ + [Array, 'array', %w[fragile gift], 'fragile,gift'], + [:array, 'array', [1, 2, 3], 7], + [Hash, 'object', { gift: true, note: 'handle with care' }, 'gift=true'], + [:object, 'object', { priority: 2 }, %w[priority 2]] + ].each do |type, json_type, valid, invalid| + it "advertises #{type.inspect} as #{json_type}, accepts one and rejects anything else" do + received = [] + server = server_with({ order_id: Integer, value: type }, received) + + expect(advertised_type(server, :value)).to eq(json_type) + expect(advertised_type(server, :order_id)).to eq('integer') + expect(server.list_tools.first.dig(:inputSchema, :required)).to eq(%w[order_id value]) + + accepted = call_tool(server, { order_id: 7, value: valid }) + expect(accepted.dig(:result, :isError)).to be(false), accepted.inspect + expect(received).to eq([{ order_id: 7, value: valid }]) + + rejected = call_tool(server, { order_id: 7, value: invalid }) + expect(rejected.dig(:result, :isError)).to be true + expect(rejected.dig(:result, :content, 0, :text)).to match(%r{Invalid arguments.*/value}m) + expect(received.size).to eq(1) + end + end + + it 'advertises the same types through the mcp gem\'s own tools/list' do + server = server_with({ tags: Array, opts: Hash, ids: :array, meta: :object }) + + response = JSON.parse(server.handle_json(JSON.generate(jsonrpc: '2.0', id: 1, method: 'tools/list')), + symbolize_names: true) + properties = response.dig(:result, :tools, 0, :inputSchema, :properties) + + expect(properties.transform_values { |property| property[:type] }) + .to eq(tags: 'array', opts: 'object', ids: 'array', meta: 'object') + expect(server.list_tools.first.dig(:inputSchema, :properties)).to eq(properties) + end + + it 'accepts an empty Array and an empty Hash' do + received = [] + server = server_with({ tags: Array, opts: Hash }, received) + + response = call_tool(server, { tags: [], opts: {} }) + + expect(response.dig(:result, :isError)).to be false + expect(received).to eq([{ tags: [], opts: {} }]) + end + + # Everything else in a shorthand schema keeps its present meaning. What an + # unrecognized value should do (warn, raise) is a separate decision; until + # then it is advertised as a string, exactly as before. + it 'leaves every other shorthand value as it was' do + schema = { name: String, count: :integer, ratio: Float, flag: :boolean, + typo: :interger, short: :bool, numeric: Numeric, + list_of: [String], fragment: { type: 'array', items: { type: 'string' } } } + + types = server_with(schema).list_tools.first.dig(:inputSchema, :properties).transform_values { |p| p[:type] } + + expect(types).to eq(name: 'string', count: 'integer', ratio: 'number', flag: 'boolean', + typo: 'string', short: 'string', numeric: 'string', + list_of: 'string', fragment: 'string') + end + + it 'still reads { type: :object } as a prebuilt schema, not as a parameter named type' do + # :object is now a shorthand value too; a propertyless object schema has + # always been detected first and must stay "accept any object". + server = server_with({ type: :object }) + + expect(server.list_tools.first[:inputSchema]).to eq({ type: 'object' }) + expect(call_tool(server, { anything: 1 }).dig(:result, :isError)).to be false + end + + it 'declares an object parameter literally named type through the class form' do + schema = server_with({ type: Hash, name: String }).list_tools.first[:inputSchema] + + expect(schema).to eq(type: 'object', required: %w[type name], + properties: { type: { type: 'object' }, name: { type: 'string' } }) + end +end