Repository navigation
Move Hash-form option interpretation into one private module (OptionForms) - #167
Merged
Merged
Conversation
Seven options take a typed value or the equivalent Hash, and fifteen readers across CommandBuilder, the root extractors, the transport and Query each interpret such a Hash with a key rule of their own (truthy fallback, presence, tag-spelling precedence, any-true). The next commits move that interpretation into one private module without changing any of it, so the behaviour has to be pinned first, at the seams a caller can observe. option_forms_characterization_spec.rb records, on main: - the whole command line for every form of system_prompt, tools, sandbox, task_budget, thinking, output_format, mcp_servers and plugins, including the both-spellings and nil / false corner cases and the collision the configured-defaults merge produces - the class, message and raise site (command_builder.rb, file only) of the four build errors - that a process requiring only claude_agent_sdk/command_builder builds the default command line - the initialize request for agents and the system prompt metadata through a fake transport, the connect -> read_started -> error order of the strict agent error, and that the metadata path never checks a custom prompt's text - that an option Hash is stored and read back as the caller's object and re-read by every build sandbox_unavailable_warning_spec.rb gains the any-true cases of the warning predicate. No production code changes. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KzJcBPQ8Vq4kjkxdm2VnYm
An option given as a Hash is interpreted in fifteen places today, each with its own rule for the spelling of a key and for nil / false under it. OptionForms gathers those rules behind stateless per-value functions: a caller hands over the value it holds and gets back a small frozen record (Prompt, Thinking, Plugin) or the value as it was given. Each function keeps the rule its reader had (truthy fallback, presence, tag-spelling precedence, any-true); nothing is unified, validated, cached or hydrated through a public constructor, and agents stay strict through AgentDefinition.new. Every read of a Hash key sits in the nested, private HashForm; the functions tell the forms apart by class and go there for the Hash one. That split also keeps the module inside Metrics/ModuleLength without an inline disable. The tools preset is answered as an object of its own, DEFAULT_TOOLS, rather than the Symbol :default: `tools: :default` is a value a caller can write, and it sends no flag today. Nothing calls the module yet. command_builder.rb requires it directly, next to its other requires, so that a process loading that file alone keeps building once the readers are switched. The module is @api private and has no signature. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KzJcBPQ8Vq4kjkxdm2VnYm
CommandBuilder interpreted a Hash-form option itself at eleven places: the type tag, the fields of a system prompt, thinking config, task budget and plugin, the tools preset, the json_schema wrapper, the sandbox section and the sdk MCP server entries. Each append stage now asks OptionForms what the option stands for and keeps what is its own: flag names, path conversion, CSV / JSON encoding and the four errors, which are still raised from this file (custom prompt without text, enabled thinking without a budget, unknown thinking config, unknown plugin type). #hash_type, #append_hash_system_prompt, #thinking_fields, #sandbox_section and #output_schema are gone, with the Hash arms of #append_tools, #append_mcp_servers, #append_plugins and #append_budget. The Hash branches that remain in #append_tools and #append_mcp_servers only choose an encoding. No behaviour changes: the characterization suite passes unchanged, including the exact command lines for the both-spellings and nil / false cases, the raise sites, the standalone require and the rebuild after a caller mutates its Hash. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KzJcBPQ8Vq4kjkxdm2VnYm
Four readers outside CommandBuilder still interpreted an option Hash themselves, and one of them carried a comment saying it had to match CommandBuilder's rule by hand: - ClaudeAgentSDK.extract_sdk_mcp_servers, .extract_exclude_dynamic_sections and .extract_system_prompt_snapshot keep their signatures and their two call sites and become one-line delegates. The sdk entries whose live server a session registers and the ones whose instance the command line strips are now recognized by the same private helper. - Query#initialize_protocol builds an agent Hash through OptionForms.agent_definition, at the same line: still strict AgentDefinition.new, still after the read loop started and before anything is written. - SubprocessCLITransport#sandbox_requested? asks OptionForms.sandbox_requested?, the same shallow any-true read, so nothing new runs on the stderr threads. The root entry point requires option_forms itself, since it now calls it directly. No behaviour changes; the characterization suite and the wiring specs (agents_hash_option, option_hash_symbol_type, sdk_mcp_server, sandbox_unavailable_warning) pass unchanged. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KzJcBPQ8Vq4kjkxdm2VnYm
The group in sandbox_hash_keys_spec.rb that walks every field of the three sandbox classes in every spelling (21 examples) built a whole command line and parsed its --settings JSON to compare one section. It now asserts on that section where it is made, OptionForms.sandbox, under the Symbol keys the typed classes write. No example is removed. The groups that pin the command line itself are unchanged: the same sandbox written three ways, and everything from the SandboxKeys tables to the end of the file (unmodeled keys, values that are not rewritten, both spellings, nil under a known key, rejected shapes, typed nested values, non-mutation). No example leaves command_builder_spec.rb either: none of its Hash-form examples repeats a row of the characterization table verbatim. CHANGELOG: one Internal line under Unreleased. Nothing a gem user would notice changed. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KzJcBPQ8Vq4kjkxdm2VnYm
Before the readers moved, CommandBuilder#append_plugins read a plugin entry in this order: the path, then the type once to recognize the plugin, and, only for a plugin it refused, the type again for the error message. OptionForms.plugin collapsed the two type reads into one and carried the value in the record. For a plain Hash literal that is the same thing; for a Hash that computes its values (a default proc) it is not: the message named the first reading instead of the second, and the entry was read once instead of twice. Such a Hash fits the signature of plugins, so this was a change of behaviour in a refactor that promised none (code review round 1, blocking 1). Plugin#raw_type is now a method that looks the type up in the entry each time it is asked for, through HashForm.plugin_type, and the record keeps the entry (the #to_h of a typed SdkPluginConfig, taken once) instead of a saved value. CommandBuilder asks for it only where it raises, so the sequence is the original one again and an accepted plugin still has its type read once. The raise stays in command_builder.rb. The characterization suite gains the cases that tell the two apart, all through CommandBuilder#build and green on main: a computing Hash that is refused (message names the second reading; lookups are path, type, type), the same with the String-key fallback, an accepted one (path, type) and an accepted one without a path, plus the raise site for a computing Hash. option_forms_spec.rb pins the lazy read and the single #to_h at the module. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KzJcBPQ8Vq4kjkxdm2VnYm
Comments only; no code changes. - SubprocessCLITransport#sandbox_requested? still said that sandbox Hashes "are forwarded to the CLI verbatim". That predates this branch and stopped being true when SandboxKeys began renaming a Hash's keys. The comment now says what the predicate is (the shallow any-true read of OptionForms.sandbox_requested?, made for the warning alone) and that the payload the CLI is sent is OptionForms.sandbox. - OptionForms and its nested HashForm claimed to be the one place a key of an option Hash is read, with nothing else in the SDK reading them. HashForm holds the direct field lookups that were moved out of the consumers. A sandbox Hash is still read and renamed by SandboxKeys.normalize (types/option_values.rb), which OptionForms.sandbox delegates to, and an agent Hash by the strict AgentDefinition.new and the Type machinery, which OptionForms.agent_definition delegates to. Both comments now say so. - The module comment also lists what the functions answer: a frozen record, a plain value, the value as given, or a new Hash. Only the records are frozen, never a value the caller owns. From the PR-level review (non-blocking 2 and blocking 4). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KzJcBPQ8Vq4kjkxdm2VnYm
Comment only; no code changes. The comment added to SubprocessCLITransport#sandbox_requested? in 2736c64 said that SandboxKeys, of two `enabled` keys, sends the later one. SandboxKeys leaves out a known key that holds nil before it assigns it, so a later nil does not replace an earlier value: { enabled: true, 'enabled' => nil } sends true, and two nils send neither. The later key wins only among the ones that are not nil (true then false sends false, while the warning predicate still says true). The comment now says that. From PR review round 2 (blocking 1). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KzJcBPQ8Vq4kjkxdm2VnYm
ya-luotao
added a commit
that referenced
this pull request
Oct 6, 2026
Internal release: OptionForms (#167), Query::RunLifecycle (#168) and SessionAssembly (#169) each move duplicated or scattered logic into one private module; no public API or signature change. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KzJcBPQ8Vq4kjkxdm2VnYm
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.
Summary
Hash-form option interpretation (
system_prompt,thinking,tools,output_format,task_budget,plugins,sandbox,mcp_servers,agents) was re-derived at 15 policy groups acrossCommandBuilder, the root extractors,SubprocessCLITransport#sandbox_requested?andQuery#initialize_protocol, each with its own key-style rule. Several October fixes (b724e0f, 774081a, 2421298, fbaf269, 1685eff, e3f1b0c, c50c5ed) touched these option-reading policies across multiple consumers and the existing sandbox normalizer; that cluster is the motivation.This PR moves the direct field lookups into one
@api privatemodule,ClaudeAgentSDK::OptionForms(lib/claude_agent_sdk/option_forms.rb): stateless per-value functions.system_prompt,thinkingandpluginreturn small frozen records; the other functions return the selected value or a Hash projection (the caller's own Hash for non-presettools/ a bareoutput_formatschema, a new wire-keyed Hash forsandboxandmcp_servers);agent_definitionkeeps the strictAgentDefinition.newconversion.OptionForms.sandboxdelegates to the existingSandboxKeys.normalize, which stays where it was.CommandBuilderkeeps flag assembly, path conversion and JSON encoding; the three root extractors keep their signatures and delegate; the transport delegates only the shallow any-truesandbox_requested?predicate;Query#initialize_protocolcallsOptionForms.agent_definitionwhere it built the type before.No change to existing public-option behaviour. Every per-option policy is preserved as it was (truthy
||fallback where it was, presencefetchwhere it was,output_schema's tag-spelling precedence, the any-true sandbox warning predicate, strictAgentDefinition.newfor agents, the rejected plugin's second type read). Option storage,configuredefaults merging,dup_with,inspect,sig/anddocs/are untouched. Unifying the key policies is deliberately not done here; that is a separate opt-in / 2.0 proposal.How it was verified
3526021) addsspec/unit/option_forms_characterization_spec.rb(208 examples at that commit, 213 at head): exact argv for every option form in every accepted spelling, the duplicate-key and nil/false corner cases, the initialize-protocol payloads through a fake transport, the sandbox warning predicate both ways, the four build-timeArgumentErrors (class, message, raise file) and the agents error (class, message, phase, no write), raw-Hashequal?identity with rebuild-after-mutation, and a fresh-processrequire 'claude_agent_sdk/command_builder'building the exact default argv. It is green onmainbefore any production change and after every commit.rubocop:disable;rake rbs:validate rbs:testgreen (3577 examples).main'slib/and this branch's, recording argv or error class+message, a second build, the three extractors,sandbox_requested?and whether the input was left untouched; after normalizing object addresses, all 24,069 records match. This is sampled coverage, not a proof over every input: a callback-backed Hash exposed one missed second read on the plugin rejection path, fixed in6c8b637and pinned with a default-proc characterization (round 2 compared read logs for eleven callback-backed cases)..fleet/arch/): the design went through three adversarial rounds with Codex before implementation; the implementation went through two Codex code-review rounds (one blocking finding, fixed in6c8b637, then APPROVE) and a PR-level pass whose description corrections are applied here.Notes for review
OptionForms::HashFormis a nestedprivate_constantmodule holding the moved Hash reads; the flat module exceeded RuboCop's 100-line module limit and the contract forbade a new disable.OptionForms::DEFAULT_TOOLSis a frozen sentinel object rather than the Symbol:default, becausetools: :defaultsends no flag onmainand must keep doing so.refactor/session-assembly): that branch deletes the two call sites of the root extractors; this branch only changes their bodies. Whichever lands second rebases.🤖 Generated with Claude Code
https://claude.ai/code/session_01KzJcBPQ8Vq4kjkxdm2VnYm