Repository navigation
Fix CLI installer defects (fiber deadlock, proxy, read-only dir, fsync) and SDK MCP schema/result bugs - #152
Merged
Merged
Conversation
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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LRaNU3CNmsGrJ9GM2hhRPC
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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LRaNU3CNmsGrJ9GM2hhRPC
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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LRaNU3CNmsGrJ9GM2hhRPC
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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LRaNU3CNmsGrJ9GM2hhRPC
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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LRaNU3CNmsGrJ9GM2hhRPC
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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LRaNU3CNmsGrJ9GM2hhRPC
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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LRaNU3CNmsGrJ9GM2hhRPC
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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LRaNU3CNmsGrJ9GM2hhRPC
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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LRaNU3CNmsGrJ9GM2hhRPC
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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LRaNU3CNmsGrJ9GM2hhRPC
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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LRaNU3CNmsGrJ9GM2hhRPC
4 tasks done
Collaborator
Author
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: df68b770ac
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| file.write(chunk) | ||
| end | ||
| file.fsync |
There was a problem hiding this comment.
When power is lost shortly after installation, this fsync guarantees only the downloaded bytes and the file's initial 0600 mode because fetch_verified performs chmod(0755) later. The subsequent directory sync persists the rename but does not guarantee persistence of metadata changes on the child inode, so the published claude file can return non-executable and installed_path will reject it. Move the chmod before this sync or fsync the file again after chmod.
Useful? React with 👍 / 👎.
Collaborator
Author
There was a problem hiding this comment.
Fixed in abfebf3: fetch_verified fsyncs the file again after the chmod, before VERSION is recorded and the binary renamed into place. spec/unit/cli_installer_durability_spec.rb now pins the order (bytes synced, chmod 755, synced again, then VERSION and the renames) and fails without the new sync.
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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CNgMt2RjeBLyRP3CWFWNy2
ya-luotao
added a commit
that referenced
this pull request
Oct 3, 2026
…witch The entry said the generated initializer turns auto-memory off, while its env: line is commented out — a newly generated multi-user app is not isolated until the developer uncomments it. The entry now says exactly that (found by the Codex review of this PR). Also folds in the fixes made after the Codex reviews of #152, #156 and #157: the binary's mode is synced after its chmod; a debug_stderr: sink and the sandbox warning are contained like the stderr: callback; tag: false no longer clears a tag. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CNgMt2RjeBLyRP3CWFWNy2
Collaborator
Author
|
@codex review |
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What and why
Eleven commits, one per fix (each with a regression spec that fails without it) plus two from the review
round. Two areas.
CLIInstallerinstallfrom two fibers of oneAsyncreactor into the same directory deadlocked: the second fiberblocked the reactor thread in
flock(LOCK_EX)while the lock holder was parked on the network inside thecritical section. The lock is now polled (
LOCK_NB+sleep), which yields to a Fiber scheduler. Mutualexclusion is unchanged; threads and processes were never affected.
install/install_pinnedraisedEACCESin a read-only install directory (an image built as rootand run as another user, a read-only root filesystem) although the pinned binary was installed and intact.
When the lock file cannot be opened (
EACCES,EROFS,EPERM), a concrete version that is installed andre-hashes to its recorded checksum is returned without the lock. A dist-tag, another version or a damaged
binary raises as before.
HTTPS_PROXY/https_proxywere ignored (Net::HTTPonly readshttp_proxyunless told otherwise).The proxy is now resolved from the
httpsURL for every hop and passed explicitly, with credentials;NO_PROXYapplies as before and an environment that only setshttp_proxykeeps working.ALL_PROXYisnot read. New docs section "Proxies and custom CAs".
Http.with_response/follow_redirectwere never executed by a spec. New specs cover the HTTPS-onlyrule, the redirect budget, a missing
Location, error statuses, error wrapping anduse_ssl, behind astubbed
Net::HTTP.startwith realNet::HTTPResponseobjects. No production change.VERSIONare fsynced before they are renamed into place — the binary againafter its chmod, so its executable mode survives too (found by the Codex review) — and the directory
after it (best-effort). Before, a power loss shortly after an install could leave an empty executable
claudethat discovery kept selecting. The order of operations is unchanged.places; "a first install that fails leaves nothing behind" (it never publishes a binary, but the directory,
the lock file and — after a failed final rename — a recorded
VERSIONcan remain; pinned by a spec).The publish order, the in-lock dist-tag resolve and the in-lock sweep are untouched, so
formal/tla/CLIInstaller.tlachanges in comments only (theCrashstep now says what holds across a powerloss and what it rests on) and
formal/tla/run.shreports all sixteen configurations as expected.SDK MCP server
Array/Hash/:array/:objectparameters asstring, so themodel sent
"a,b"and the handler got a String; an actual array was rejected by validation. They now mapto
array/object, intools/listand intools/callvalidation alike (mcp 0.22.0 and 1.6.1). Everyother shorthand value behaves as before. The docs gain a table of the shorthand types.
{ content: 'text' }or a single block Hash produced a frame the CLI rejects ("serverreturned a malformed result"). It is now an in-band
isErrorresult naming the tool and the problem, onboth
tools/callpaths; a presentfalseornilis reported the same way, and a missing:contentkeykeeps its own message.
create_tool(annotations: { maxResultSizeChars: … }, meta: { … })dropped the size hint derived from theannotation. The two are merged; a key set in
meta:wins.Verification:
bundle exec rspecgreen on the lockfile,gemfiles/latest.gemfileandgemfiles/floor.gemfile; RuboCop clean;rake rbs:validate rbs:testpasses;formal/tla/run.sh(16/16 asexpected); the six-process installer probe (one download, no overlapping critical sections, lock-free readers
only ever see a complete binary); a real-CLI handshake without a model turn for the
tools/listframe. Tworounds of independent adversarial review (each production change reverted → its spec fails).
Not in this PR
install(dir: '')raising, and detecting a foreignVERSIONfile in the install directory: new errorsfor input that is accepted today — pending a maintainer decision.
non-Array
:content(reported as an error instead).install(verify:),lock_timeout:.formal/tla/run.shon a fresh checkout: the upstreamtla2tools.jarasset of thepinned v1.8.0 release was rebuilt on 2026-10-02, so its SHA-256 no longer matches the pin in
run.sh(cached jars still verify). Not changed here.
CHANGELOG.md: this PR's entries are in the batch's combined CHANGELOG PR (kept separate so the fix PRsdo not conflict with each other).
Checklist
bundle exec rakepasses (specs + RuboCop)## [Unreleased]— in the batch's combined CHANGELOG PRdocs/cli-installer.md,docs/mcp-servers.md, YARD, both skill trees,formal/tla/README.md)🤖 Generated with Claude Code
https://claude.ai/code/session_01CNgMt2RjeBLyRP3CWFWNy2