From f80384f8d856adf8aab73feacd8216d4a8104e3b Mon Sep 17 00:00:00 2001 From: ya-luotao Date: Sat, 3 Oct 2026 00:48:01 +0800 Subject: [PATCH 01/12] Poll the install lock so fibers of one reactor cannot deadlock Two fibers of one Async reactor calling CLIInstaller.install for the same directory never returned. The first took the lock and parked in a scheduler-aware wait inside the critical section (resolving the dist-tag, fetching the manifest, downloading); the second then called flock(LOCK_EX), which has no Fiber-scheduler hook and blocked the reactor thread, so the lock holder was never resumed. With the default version: 'stable' this happened even when nothing needed installing, because the tag is resolved inside the lock. Threads and processes were not affected. with_install_lock now polls: sleep(LOCK_POLL_SECONDS) until flock(LOCK_EX | LOCK_NB). Kernel#sleep yields to a Fiber scheduler and is an ordinary sleep on a plain thread. Mutual exclusion is unchanged, and so are the publish order, the in-lock dist-tag resolve and the in-lock sweep that formal/tla/CLIInstaller.tla checks (the model never assumed a fair or FIFO lock). The regression spec runs the two fibers in a child process, because a regression blocks the calling thread instead of failing. The existing thread-contention example recognized a waiter by its innermost frame being File#flock; a waiter is now asleep between attempts, so the helper looks for the poll loop instead. User-visible: a waiting installer wakes up to 50 ms after the lock is released instead of immediately. Exploration finding: D-P5 Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01LRaNU3CNmsGrJ9GM2hhRPC --- docs/cli-installer.md | 2 +- lib/claude_agent_sdk/cli_installer.rb | 21 +++- spec/unit/cli_installer_fiber_lock_spec.rb | 130 +++++++++++++++++++++ spec/unit/cli_installer_spec.rb | 14 ++- 4 files changed, 157 insertions(+), 10 deletions(-) create mode 100644 spec/unit/cli_installer_fiber_lock_spec.rb diff --git a/docs/cli-installer.md b/docs/cli-installer.md index e4ec42b4..9d812f90 100644 --- a/docs/cli-installer.md +++ b/docs/cli-installer.md @@ -27,7 +27,7 @@ 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 → place → record 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. Failures (unsupported platform, invalid version, HTTP error, response-size cap, oversized download, checksum mismatch, filesystem errors) raise `ClaudeAgentSDK::CLIInstallError`. diff --git a/lib/claude_agent_sdk/cli_installer.rb b/lib/claude_agent_sdk/cli_installer.rb index ab9a9423..123355e4 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 @@ -441,14 +446,24 @@ 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. + # 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. def with_install_lock(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) + sleep(LOCK_POLL_SECONDS) until lock.flock(File::LOCK_EX | File::LOCK_NB) begin yield ensure 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_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 From 80b1261dbec9514e4ef2be57c5b398de600e9256 Mon Sep 17 00:00:00 2001 From: ya-luotao Date: Sat, 3 Oct 2026 00:52:45 +0800 Subject: [PATCH 02/12] Let install's offline shortcut work in a read-only directory A boot-time CLIInstaller.install_pinned raised CLIInstallError (EACCES) in a directory the process cannot write, although the pinned binary was installed and intact: an image built as root and run as another user, a read-only root filesystem. The idempotent shortcut (re-hash the binary, compare with VERSION, return) sits inside the install lock, and the lock file is opened read-write, and created, before it is ever consulted. Lock-free discovery (installed_path, find_cli) kept working, so only callers that run install at boot were hit, which is what the docs recommend. install now rescues EACCES, EROFS and EPERM around opening the lock file and nothing else. When the request is a concrete version that is installed and re-hashes to its recorded checksum for this platform, it returns the binary; otherwise it raises exactly as before. A dist-tag always raises there: it has to be resolved and may have to be published, which needs the lock. A permission error raised inside the critical section still fails the install. Nothing is written and no lock is taken on that path, so it is one more lock-free reader of the kind ReadersSeeVerifiedBinary covers; the publish order, the in-lock resolve and the in-lock sweep are untouched, and the TLA model's expectations do not change (formal/tla/README.md says so). with_install_lock now takes the open lock file instead of the directory; both are private. Exploration finding: D-P7 Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01LRaNU3CNmsGrJ9GM2hhRPC --- docs/cli-installer.md | 1 + formal/tla/README.md | 1 + lib/claude_agent_sdk/cli_installer.rb | 58 +++++-- spec/unit/cli_installer_read_only_spec.rb | 183 ++++++++++++++++++++++ 4 files changed, 233 insertions(+), 10 deletions(-) create mode 100644 spec/unit/cli_installer_read_only_spec.rb diff --git a/docs/cli-installer.md b/docs/cli-installer.md index 9d812f90..dfbebdee 100644 --- a/docs/cli-installer.md +++ b/docs/cli-installer.md @@ -28,6 +28,7 @@ ClaudeAgentSDK::CLIInstaller.installed_path - 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 — 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`. diff --git a/formal/tla/README.md b/formal/tla/README.md index 0b2ad1d5..306cb209 100644 --- a/formal/tla/README.md +++ b/formal/tla/README.md @@ -45,5 +45,6 @@ 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. +- 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 123355e4..5748401c 100644 --- a/lib/claude_agent_sdk/cli_installer.rb +++ b/lib/claude_agent_sdk/cli_installer.rb @@ -391,6 +391,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) @@ -401,7 +407,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 @@ -458,18 +476,38 @@ def installed_path(dir: nil) # 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. - def with_install_lock(dir) + # + # +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| - sleep(LOCK_POLL_SECONDS) until lock.flock(File::LOCK_EX | File::LOCK_NB) - 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 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 From dec5eda95342209f36ab5aafcd2473f82d4303cd Mon Sep 17 00:00:00 2001 From: ya-luotao Date: Sat, 3 Oct 2026 00:57:51 +0800 Subject: [PATCH 03/12] Honor HTTPS_PROXY in the CLI installer CLIInstaller downloaded without the proxy in an environment that exports only HTTPS_PROXY / https_proxy, which is what curl, RubyGems and the CLI itself read for an https URL. Net::HTTP.start without proxy arguments looks the proxy up as if for an http:// URL: it reads http_proxy even for a TLS connection. Where direct egress is blocked the install failed after the 10 s open timeout, with a message that does not mention the proxy. Http.with_response now resolves the proxy with URI#find_proxy on the https URL, for every redirect hop, and passes its address, port, user and password to Net::HTTP.start explicitly; credentials are percent-decoded the way Net::HTTP decodes the ones it finds itself. no_proxy / NO_PROXY behave as find_proxy defines them. Nothing else changes route. With no usable HTTPS proxy, no arguments are passed and Net::HTTP's own lookup stays in charge, so an environment that only sets http_proxy keeps working. A value Net::HTTP cannot use as an HTTP proxy (no scheme, socks5://, https://, not a URL) was ignored before and still is, instead of turning a download that works directly into a failure. ALL_PROXY is not consulted, so this is not curl parity; docs/cli-installer.md gains a "Proxies and custom CAs" section that says so and also covers NO_PROXY and SSL_CERT_FILE. The new spec file stubs only Net::HTTP.start and answers with real Net::HTTPResponse objects parsed from raw bytes. User-visible: a host that sets https_proxy to an http:// proxy now downloads the CLI through it. Exploration finding: D-P6 Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01LRaNU3CNmsGrJ9GM2hhRPC --- docs/cli-installer.md | 11 ++ lib/claude_agent_sdk/cli_installer.rb | 35 ++++++- spec/unit/cli_installer_http_spec.rb | 145 ++++++++++++++++++++++++++ 3 files changed, 188 insertions(+), 3 deletions(-) create mode 100644 spec/unit/cli_installer_http_spec.rb diff --git a/docs/cli-installer.md b/docs/cli-installer.md index dfbebdee..38f30fdc 100644 --- a/docs/cli-installer.md +++ b/docs/cli-installer.md @@ -97,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/lib/claude_agent_sdk/cli_installer.rb b/lib/claude_agent_sdk/cli_installer.rb index 5748401c..dec1c3bb 100644 --- a/lib/claude_agent_sdk/cli_installer.rb +++ b/lib/claude_agent_sdk/cli_installer.rb @@ -200,9 +200,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. @@ -218,6 +218,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 diff --git a/spec/unit/cli_installer_http_spec.rb b/spec/unit/cli_installer_http_spec.rb new file mode 100644 index 00000000..6840a2a5 --- /dev/null +++ b/spec/unit/cli_installer_http_spec.rb @@ -0,0 +1,145 @@ +# frozen_string_literal: true + +require 'spec_helper' +require 'net/http' +require 'stringio' + +# 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 +end From f408355aa7e3c5baa9e372795897021d411223fc Mon Sep 17 00:00:00 2001 From: ya-luotao Date: Sat, 3 Oct 2026 01:00:53 +0800 Subject: [PATCH 04/12] Test CLIInstaller::Http's HTTPS-only rule, redirects and failures Nothing executed Http.with_response and Http.follow_redirect: the installer specs replace Http.fetch_text / Http.download_to wholesale. Deleting the "Refusing to fetch non-HTTPS URL" check, or letting the redirect budget never decrease, left the whole suite green. The Http spec file now covers them with canned responses behind a stubbed Net::HTTP.start (stdlib only; the responses are real Net::HTTPResponse objects parsed from raw bytes): - an http:// URL is refused before any connection is opened; - a redirect from https to http is refused, not followed; - five redirects are followed, the sixth raises "Too many redirects" after exactly six requests; - a relative Location is resolved against the URL that sent it, and a redirect without Location raises; - a 404 raises with its status line, and an error page never creates the target file; a redirect body is never streamed into it; - a SocketError is wrapped in CLIInstallError with cause, while a CLIInstallError from the body handler passes through unwrapped; - every hop is opened with use_ssl: true and the timeouts, and with no option that weakens peer verification. That option, not the scheme check, is what keeps the transfer encrypted: with the check removed, Net::HTTP still speaks TLS to an http:// URL's host. No production change. Each of these mutants now fails the file: the scheme check deleted, the budget not decremented, use_ssl dropped, the status tested after the body is handed over, the missing-Location check deleted, socket errors left unwrapped. Exploration finding: F-P6 Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01LRaNU3CNmsGrJ9GM2hhRPC --- spec/unit/cli_installer_http_spec.rb | 148 +++++++++++++++++++++++++++ 1 file changed, 148 insertions(+) diff --git a/spec/unit/cli_installer_http_spec.rb b/spec/unit/cli_installer_http_spec.rb index 6840a2a5..407cf2d2 100644 --- a/spec/unit/cli_installer_http_spec.rb +++ b/spec/unit/cli_installer_http_spec.rb @@ -3,6 +3,7 @@ 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 @@ -142,4 +143,151 @@ def proxy_used_for(url) 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 From 4d93021447cb897444d92e793c2f3403c3d06fb0 Mon Sep 17 00:00:00 2001 From: ya-luotao Date: Sat, 3 Oct 2026 01:04:23 +0800 Subject: [PATCH 05/12] Advertise Array and Hash shorthand tool parameters correctly A shorthand tool schema such as { order_id: Integer, tags: Array } told the model that tags is a string: Array, Hash, :array and :object all fell into the default branch of ruby_type_to_json_schema. In a real session the model then sent "fragile,gift" and the handler received a String where it had declared an Array; a model that did send an array had the call rejected by argument validation ("value at `/tags` is not a string"). Array / :array now map to { type: 'array' } and Hash / :object to { type: 'object' }, the same kind of fix 8e3419c made for Integer, Float and the booleans. tools/list and the tools/call validation both derive from normalize_tool_schema, so they agree: an array or object argument is accepted and reaches the handler, anything else is rejected in-band. Checked on mcp 0.22.0 and 1.6.1. Every other shorthand value keeps its present meaning: an unrecognized Symbol or class, an Array value such as [String] and a nested schema fragment are still advertised as a string, with no warning and no error (that policy is a separate decision). { type: :object } is still read as a prebuilt accept-any-object schema, as it always was; a parameter literally named type is declared with { type: Hash }. docs/mcp-servers.md had no description of the shorthand at all; it gains a short table of the accepted types. User-visible: a tool that declared Array or Hash and coped with the String it used to receive now receives an Array or a Hash. Exploration finding: D-P8 (= U-P18) Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01LRaNU3CNmsGrJ9GM2hhRPC --- docs/mcp-servers.md | 21 +++++ lib/claude_agent_sdk/sdk_mcp_server.rb | 16 +++- spec/unit/sdk_mcp_tool_schema_spec.rb | 112 +++++++++++++++++++++++++ 3 files changed, 147 insertions(+), 2 deletions(-) create mode 100644 spec/unit/sdk_mcp_tool_schema_spec.rb diff --git a/docs/mcp-servers.md b/docs/mcp-servers.md index 296cbb6b..8e7b0a36 100644 --- a/docs/mcp-servers.md +++ b/docs/mcp-servers.md @@ -58,6 +58,27 @@ 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. +## 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 If your schemas come from another library (e.g., [RubyLLM](https://github.com/crmne/ruby_llm)) that deep-stringifies keys, the SDK handles them transparently — both symbol-keyed and string-keyed schemas are accepted and normalized: diff --git a/lib/claude_agent_sdk/sdk_mcp_server.rb b/lib/claude_agent_sdk/sdk_mcp_server.rb index cdb099e3..9a474887 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) @@ -68,12 +71,16 @@ def self.normalize_tool_schema(schema) 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) + 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 @@ -606,7 +613,12 @@ 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 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+ / 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 From 5330264308c4e33e8125352ef7d24285335244a2 Mon Sep 17 00:00:00 2001 From: ya-luotao Date: Sat, 3 Oct 2026 01:06:59 +0800 Subject: [PATCH 06/12] Report a non-Array tool :content as an in-band error MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A tool handler that returned { content: 'text' } — an easy slip next to the String form — or a single block Hash had its result forwarded as it was. The SDK only checked that the Hash has a :content key. The CLI then rejected the frame against its schema, and the model was told that the server "returned a malformed result that failed schema validation"; the text never reached it. Both tools/call paths (the mcp gem's tool class and the direct SdkMcpServer#call_tool) now answer such a result with an isError result that names the tool and says what is wrong: Tool 'order_status' must return :content as an Array of content blocks (got String) The legal shapes are unchanged: a String, and a Hash whose :content is an Array (an empty one included). The result is not wrapped into a text block automatically; that would be a new accepted form and is left as a separate decision. create_tool_classes builds the dynamic tool class inline, so the extra guard raised its complexity count by one; the two cops join the named disable the method already carries. Exploration finding: D-P9 Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01LRaNU3CNmsGrJ9GM2hhRPC --- docs/mcp-servers.md | 2 +- lib/claude_agent_sdk/sdk_mcp_server.rb | 15 +++++- spec/unit/sdk_mcp_tool_result_spec.rb | 73 ++++++++++++++++++++++++++ 3 files changed, 88 insertions(+), 2 deletions(-) create mode 100644 spec/unit/sdk_mcp_tool_result_spec.rb diff --git a/docs/mcp-servers.md b/docs/mcp-servers.md index 8e7b0a36..170aba49 100644 --- a/docs/mcp-servers.md +++ b/docs/mcp-servers.md @@ -56,7 +56,7 @@ 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 diff --git a/lib/claude_agent_sdk/sdk_mcp_server.rb b/lib/claude_agent_sdk/sdk_mcp_server.rb index 9a474887..c6e5aa38 100644 --- a/lib/claude_agent_sdk/sdk_mcp_server.rb +++ b/lib/claude_agent_sdk/sdk_mcp_server.rb @@ -311,6 +311,11 @@ def call_tool(name, arguments) # 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 + # 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 @@ -419,7 +424,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 @@ -506,6 +511,14 @@ def call(server_context: nil, **args) # rubocop:disable Lint/UnusedMethodArgumen 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') 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..598254b9 --- /dev/null +++ b/spec/unit/sdk_mcp_tool_result_spec.rb @@ -0,0 +1,73 @@ +# 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'] + ].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 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 From accd6557266d30a2dbfefd9f8858566689632e78 Mon Sep 17 00:00:00 2001 From: ya-luotao Date: Sat, 3 Oct 2026 01:10:08 +0800 Subject: [PATCH 07/12] Merge create_tool's meta: with the size hint from annotations create_tool(annotations: { maxResultSizeChars: 500_000 }, meta: { 'acme/team' => 'billing' }) advertised _meta as { 'acme/team' => 'billing' }: the size hint derived from the annotation was only added when no meta: was passed, so any explicit meta:, even one about something else, dropped it. The annotation itself is not read by MCP clients; _meta 'anthropic/maxResultSizeChars' is what raises the CLI's inline result size, so large results went back to being persisted and previewed, with no diagnostic. The derived key is now merged with the explicit meta:. A size key the caller sets wins, as a String or as a Symbol; in the Symbol case the String key is not added beside it, which would put the same JSON key in the frame twice (json 3.x refuses to generate that). The Hash passed as meta: is not modified. Without a size annotation meta: is passed through as before. Reflows the class map in ruby_type_to_json_schema onto two lines to stay inside the module-length budget. Exploration finding: D-P12 Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01LRaNU3CNmsGrJ9GM2hhRPC --- lib/claude_agent_sdk/sdk_mcp_server.rb | 21 +++-- .../skills/claude-agent-ruby/SKILL.md | 2 +- skills/SKILL.md | 2 +- spec/unit/sdk_mcp_tool_meta_spec.rb | 85 +++++++++++++++++++ 4 files changed, 102 insertions(+), 8 deletions(-) create mode 100644 spec/unit/sdk_mcp_tool_meta_spec.rb diff --git a/lib/claude_agent_sdk/sdk_mcp_server.rb b/lib/claude_agent_sdk/sdk_mcp_server.rb index c6e5aa38..94cbcf3f 100644 --- a/lib/claude_agent_sdk/sdk_mcp_server.rb +++ b/lib/claude_agent_sdk/sdk_mcp_server.rb @@ -70,9 +70,8 @@ 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, - Array => :array, Hash => :object }.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' } @@ -632,6 +631,11 @@ def convert_input_schema(schema) # +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+ / @@ -666,11 +670,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/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 From c48b5ffea5acf2d5b8d3e68735de1b499b7b3740 Mon Sep 17 00:00:00 2001 From: ya-luotao Date: Sat, 3 Oct 2026 01:17:19 +0800 Subject: [PATCH 08/12] Fsync the installed CLI and its VERSION before publishing them Nothing was fsynced before the installer renamed the downloaded binary into place. rename() is atomic for concurrent readers and across a process crash, but across a power loss it only orders metadata: the directory could come back holding a zero-length or partial executable `claude` next to a VERSION naming it. Lock-free discovery (installed_path checks only "file and executable") then handed that file to every spawn until something called install again. The TLA model nevertheless listed power loss among the things its Crash step covers. Now: - Http.download_to fsyncs the temp file before closing it; - Metadata.write fsyncs the VERSION temp file before renaming it; - #publish fsyncs the directory after the binary's rename, best-effort. It runs where nothing may fail the install, and not every platform or filesystem can sync a directory, so SystemCallError, IOError and NotImplementedError are swallowed there. The order of operations is unchanged (download, verify, record VERSION, rename last), and the rename is still the last step that can fail. A failed fsync of the download or of VERSION fails the install like any other write error, before anything is published; the previous install stays intact. The Crash comment in formal/tla/CLIInstaller.tla now says what holds for a power loss and what it rests on (those fsyncs, fsync(2) reaching stable storage, ordered directory operations), and names what is not modelled; formal/tla/README.md lists durability among the assumptions. Comments only: no modelled step and no run.sh expectation changes. User-visible: an install that downloads pays for one fsync of the binary. The idempotent shortcut writes nothing and syncs nothing. Exploration finding: D-P11 Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01LRaNU3CNmsGrJ9GM2hhRPC --- docs/cli-installer.md | 2 +- formal/tla/CLIInstaller.tla | 21 ++- formal/tla/README.md | 1 + lib/claude_agent_sdk/cli_installer.rb | 28 ++++ spec/unit/cli_installer_durability_spec.rb | 172 +++++++++++++++++++++ 5 files changed, 222 insertions(+), 2 deletions(-) create mode 100644 spec/unit/cli_installer_durability_spec.rb diff --git a/docs/cli-installer.md b/docs/cli-installer.md index 38f30fdc..c03e913b 100644 --- a/docs/cli-installer.md +++ b/docs/cli-installer.md @@ -32,7 +32,7 @@ ClaudeAgentSDK::CLIInstaller.installed_path 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 leaves nothing behind at all. ## When the pin moves diff --git a/formal/tla/CLIInstaller.tla b/formal/tla/CLIInstaller.tla index 86ccfdf4..766ddb8d 100644 --- a/formal/tla/CLIInstaller.tla +++ b/formal/tla/CLIInstaller.tla @@ -183,7 +183,26 @@ 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 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 306cb209..a3f7bb02 100644 --- a/formal/tla/README.md +++ b/formal/tla/README.md @@ -45,6 +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 dec1c3bb..4182495a 100644 --- a/lib/claude_agent_sdk/cli_installer.rb +++ b/lib/claude_agent_sdk/cli_installer.rb @@ -173,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 @@ -185,6 +190,7 @@ def download_to(url, path, max_bytes: nil) file.write(chunk) end + file.fsync end end path @@ -356,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 @@ -585,17 +594,36 @@ 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) 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 or partial 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 diff --git a/spec/unit/cli_installer_durability_spec.rb b/spec/unit/cli_installer_durability_spec.rb new file mode 100644 index 00000000..03072099 --- /dev/null +++ b/spec/unit/cli_installer_durability_spec.rb @@ -0,0 +1,172 @@ +# 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. +# +# 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] and + # [:rename, from, to], 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 its fsync raises instead. + def spy_on_syncs_and_renames(failing: {}) + spy_on_fsync(failing) + log = events + labeller = method(:label) + allow(File).to receive(:rename).and_wrap_original do |original, from, to| + original.call(from, to) + log << [:rename, labeller.call(from), labeller.call(to)] + 0 + 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 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 + + 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], + [: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 + + 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_failed, 'VERSION..tmp']]) + expect_previous_install_intact + end + end +end From f8f0a5c72843905a965ccb17547e14c24c2a8703 Mon Sep 17 00:00:00 2001 From: ya-luotao Date: Sat, 3 Oct 2026 01:21:37 +0800 Subject: [PATCH 09/12] Correct installer and SDK MCP comments that contradict the code Text fixes, plus one error message: - docs/cli-installer.md and install's YARD described the locked sequence as check, download, place, record. The code records VERSION and then places the binary, deliberately: that order is what keeps a failed upgrade from destroying a working install, and the paragraph a few lines below already said so. - docs/cli-installer.md said a first install that fails "leaves nothing behind at all". It leaves no binary and no VERSION, but the install directory and its lock file can remain. - The comment on the tool input schema said the mcp gem validates against the draft4 metaschema and that a numeric exclusiveMinimum or $ref/$defs falls back to a permissive schema. Every mcp version this gem supports (0.22 and later) validates against JSON Schema 2020-12: those schemas are accepted and validated. The fallback is still reachable, for what that metaschema refuses (a draft4-style boolean exclusiveMinimum, an unknown type, a $ref that leaves the document, an invalid pattern); the comment now names those. - When the working directory has been deleted, resolving the default install directory raises before `dir` is assigned, and the wrapped error read "Failed to install the Claude Code CLI into : Errno::ENOENT ... getcwd". It now names the default directory by its relative path (vendor/claude). The fifth comment of this kind, "threads contend like processes" on the install lock, was corrected with the polling lock. Exploration finding: D-P15 Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01LRaNU3CNmsGrJ9GM2hhRPC --- docs/cli-installer.md | 4 +-- lib/claude_agent_sdk/cli_installer.rb | 9 +++-- lib/claude_agent_sdk/sdk_mcp_server.rb | 26 ++++++++------ spec/unit/cli_installer_error_message_spec.rb | 34 +++++++++++++++++++ 4 files changed, 58 insertions(+), 15 deletions(-) create mode 100644 spec/unit/cli_installer_error_message_spec.rb diff --git a/docs/cli-installer.md b/docs/cli-installer.md index c03e913b..171dfa09 100644 --- a/docs/cli-installer.md +++ b/docs/cli-installer.md @@ -27,12 +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 — 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. +- 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. 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 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 leaves no binary and no `VERSION` behind — at most the install directory and its `.install.lock`. ## When the pin moves diff --git a/lib/claude_agent_sdk/cli_installer.rb b/lib/claude_agent_sdk/cli_installer.rb index 4182495a..817ea9cc 100644 --- a/lib/claude_agent_sdk/cli_installer.rb +++ b/lib/claude_agent_sdk/cli_installer.rb @@ -419,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. @@ -471,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 diff --git a/lib/claude_agent_sdk/sdk_mcp_server.rb b/lib/claude_agent_sdk/sdk_mcp_server.rb index 94cbcf3f..32eba563 100644 --- a/lib/claude_agent_sdk/sdk_mcp_server.rb +++ b/lib/claude_agent_sdk/sdk_mcp_server.rb @@ -455,16 +455,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? 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 From 6ac34e19525cdbb2fe6c9ac6734f36887dd22888 Mon Sep 17 00:00:00 2001 From: ya-luotao Date: Sat, 3 Oct 2026 01:58:49 +0800 Subject: [PATCH 10/12] Say what a failed first install can really leave behind docs/cli-installer.md said a first install that fails "leaves no binary and no VERSION behind". The second half is wrong for one case. VERSION is recorded before the binary is renamed into place (that order is what protects a working install during an upgrade, and it does not change), so when the step that fails is the last one, the rename, the directory is left with .install.lock and a complete VERSION and no binary. The paragraph now says so: a failed first install never publishes a binary; it can leave the directory, the lock file and, after a failed rename, a VERSION that already records the version. That state is harmless: installed? re-hashes the binary rather than trusting VERSION, so lock-free discovery finds no binary and moves on, and the next install redoes it. Three examples in the durability spec pin the wording: a first install whose rename fails (VERSION recorded, no binary, installed_path nil), the next install redoing it, and an earlier failure leaving only the lock file. No production change. Review finding: install-r1 B1 (follow-up to D-P15) Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01LRaNU3CNmsGrJ9GM2hhRPC --- docs/cli-installer.md | 2 +- spec/unit/cli_installer_durability_spec.rb | 72 +++++++++++++++++++--- 2 files changed, 66 insertions(+), 8 deletions(-) diff --git a/docs/cli-installer.md b/docs/cli-installer.md index 171dfa09..cd97e030 100644 --- a/docs/cli-installer.md +++ b/docs/cli-installer.md @@ -32,7 +32,7 @@ ClaudeAgentSDK::CLIInstaller.installed_path 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. 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 leaves no binary and no `VERSION` behind — at most the install directory and its `.install.lock`. +**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 diff --git a/spec/unit/cli_installer_durability_spec.rb b/spec/unit/cli_installer_durability_spec.rb index 03072099..aa3a869f 100644 --- a/spec/unit/cli_installer_durability_spec.rb +++ b/spec/unit/cli_installer_durability_spec.rb @@ -20,9 +20,9 @@ let(:metadata) { ClaudeAgentSDK::CLIInstaller::Metadata } let(:dir) { @dir } let(:binary_path) { File.join(dir, 'claude') } - # [:fsync, name, size at that moment], [:fsync_failed, name] and - # [:rename, from, to], in order; the random part of a temp name is replaced - # by . + # [:fsync, name, size at that moment], [:fsync_failed, name], + # [:rename, from, to] and [:rename_failed, from, to], in order; the random + # part of a temp name is replaced by . let(:events) { [] } around do |example| @@ -39,15 +39,24 @@ def label(path) end # Records every fsync and every rename, then lets the real one run. - # +failing+ maps a label to the error its fsync raises instead. + # +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| - original.call(from, to) - log << [:rename, labeller.call(from), labeller.call(to)] - 0 + 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 @@ -135,6 +144,55 @@ def stub_release(version:, body:) 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 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') From df68b770ac32e480cdec7271a43c792675d85d06 Mon Sep 17 00:00:00 2001 From: ya-luotao Date: Sat, 3 Oct 2026 02:01:22 +0800 Subject: [PATCH 11/12] Tell a missing tool :content key from a present non-Array value A handler result with a PRESENT but falsy :content got the wrong diagnostic, and the two tools/call paths disagreed about it: { content: false } direct call_tool: "must return a hash with :content key" routed (mcp gem): "must return :content as an Array of content blocks (got FalseClass)" { content: nil } both paths: "must return a hash with :content key" The direct path tested the value for truthiness and the routed path for nil, so each called a key that is there "missing". Both paths now ask about the key first (result.key?(:content) || result.key?('content'), after the Hash check), and only then check that the value is an Array. A result that is not a Hash, or a Hash without the key under either spelling, still gets "must return a hash with :content key"; a present false or nil gets the Array diagnostic with its class, like any other non-Array value. Legal results are unchanged, including a Hash that carries the Array under one spelling and nil under the other. Review finding: install-r1 B2 (follow-up to D-P9) Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01LRaNU3CNmsGrJ9GM2hhRPC --- lib/claude_agent_sdk/sdk_mcp_server.rb | 21 ++++++++++------ spec/unit/sdk_mcp_tool_result_spec.rb | 34 +++++++++++++++++++++++++- 2 files changed, 47 insertions(+), 8 deletions(-) diff --git a/lib/claude_agent_sdk/sdk_mcp_server.rb b/lib/claude_agent_sdk/sdk_mcp_server.rb index 32eba563..cac87fb9 100644 --- a/lib/claude_agent_sdk/sdk_mcp_server.rb +++ b/lib/claude_agent_sdk/sdk_mcp_server.rb @@ -307,9 +307,14 @@ 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 " \ @@ -510,12 +515,14 @@ 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. diff --git a/spec/unit/sdk_mcp_tool_result_spec.rb b/spec/unit/sdk_mcp_tool_result_spec.rb index 598254b9..ee3fde84 100644 --- a/spec/unit/sdk_mcp_tool_result_spec.rb +++ b/spec/unit/sdk_mcp_tool_result_spec.rb @@ -34,7 +34,12 @@ def expect_content_error(result, got) ['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'] + ['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) @@ -51,6 +56,33 @@ def expect_content_error(result, got) 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' }]], From abfebf3ee4196d6e4ee965ee70bb8d3f7c1eb9cd Mon Sep 17 00:00:00 2001 From: ya-luotao Date: Sat, 3 Oct 2026 10:26:55 +0800 Subject: [PATCH 12/12] Sync the CLI binary's mode after its chmod MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The installer fsyncs the downloaded bytes in Http.download_to, and only then (in fetch_verified, after the checksum) chmods the file to 0755. The directory sync after the rename persists the rename, not the file's own inode metadata, so after a power loss the published `claude` could come back without its executable bit — and installed_path skips a file that is not executable. The file is fsynced once more after the chmod, before VERSION is recorded and the binary renamed into place. The durability spec pins the order (bytes synced, chmod, synced again, then VERSION and the renames) with a chmod spy; the two sequence assertions that counted the syncs include the new one. The CLIInstaller.tla comment on what the code fsyncs says the same; the publish order the model checks is unchanged (formal/tla/run.sh: every configuration as expected). Found by the Codex review of #152. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01CNgMt2RjeBLyRP3CWFWNy2 --- formal/tla/CLIInstaller.tla | 5 +++-- lib/claude_agent_sdk/cli_installer.rb | 12 +++++++++-- spec/unit/cli_installer_durability_spec.rb | 24 +++++++++++++++++----- 3 files changed, 32 insertions(+), 9 deletions(-) diff --git a/formal/tla/CLIInstaller.tla b/formal/tla/CLIInstaller.tla index 766ddb8d..5fe752bf 100644 --- a/formal/tla/CLIInstaller.tla +++ b/formal/tla/CLIInstaller.tla @@ -189,8 +189,9 @@ Fail(i) == \* \* 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 before Rename -\* and the VERSION bytes before Record's rename. A name that survives the +\* 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` diff --git a/lib/claude_agent_sdk/cli_installer.rb b/lib/claude_agent_sdk/cli_installer.rb index 817ea9cc..c28bb6ce 100644 --- a/lib/claude_agent_sdk/cli_installer.rb +++ b/lib/claude_agent_sdk/cli_installer.rb @@ -599,9 +599,11 @@ def installed?(dir, version, platform) # 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) and the VERSION bytes (Metadata.write) are + # 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 or partial one. The + # 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. @@ -639,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/spec/unit/cli_installer_durability_spec.rb b/spec/unit/cli_installer_durability_spec.rb index aa3a869f..cc3d486c 100644 --- a/spec/unit/cli_installer_durability_spec.rb +++ b/spec/unit/cli_installer_durability_spec.rb @@ -10,7 +10,8 @@ # 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. +# 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. @@ -21,8 +22,9 @@ let(:dir) { @dir } let(:binary_path) { File.join(dir, 'claude') } # [:fsync, name, size at that moment], [:fsync_failed, name], - # [:rename, from, to] and [:rename_failed, from, to], in order; the random - # part of a temp name is replaced by . + # [: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| @@ -60,6 +62,14 @@ def spy_on_rename(failing) 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) @@ -89,16 +99,19 @@ def stub_release(version:, body:) allow(http).to receive(:with_response) { |_url, &handler| handler.call(response) } end - it 'syncs the download and the metadata before their renames, and the directory after the last one' do + 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'], @@ -176,7 +189,7 @@ def stub_release(version:, body:) expect(described_class.install(version: '2.1.220', dir: dir)).to eq(binary_path) - expect(events.map(&:first)).to eq(%i[fsync fsync rename rename fsync]) + 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 @@ -223,6 +236,7 @@ def expect_previous_install_intact .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