Merged
Conversation
bfe4b72 to
a8dfe47
Compare
bananabot9000
approved these changes
Apr 8, 2026
Collaborator
bananabot9000
left a comment
There was a problem hiding this comment.
Well-executed config extraction 🍌 +553/-141 across 27 files, clean architecture.
Highlights:
io: 'input'ontoJSONSchema()-- the right fix, killed thecleanSchemahackadditivePathsgeneralization onmergeRawConfigs-- zero knowledge of specific fields- Zod
.catch()chains for fault tolerance -- invalid config falls back to defaults SdkConfigWatcherencapsulation -- private fields, construct/use/dispose lifecycle- Turn-latch pattern for model display -- prevents showing next-turn model during current execution
Items to note:
- Two
astype casts onloadConfigreturn -- may be hiding mismatch from$schemaOmit. Worth checking if the cast is actually needed or masking a type error. SdkConfigWatcherhas zero test coverage -- most stateful new code (debounce, dispose, reload failure, missing-file edge case). Main risk area.- Watcher only watches files existing at construction time -- creating config file after startup won't trigger hot reload. Acceptable for v0, worth documenting.
- Session log bundles context from PRs #219/#220/#222 -- not a blocker.
cli-config.schema.jsonlostrequiredconstraints on nested objects due toio: 'input'-- intentional but decreased validation strictness.
No sensitive data, no reversions.
Reviewed by BananaBot9000 🍌
…-sdk-cli mergeRawConfigs, loadConfig, cleanSchema, and generateJsonSchema move to claude-core/src/config.ts so both apps can share them without duplication. The execPermissions.approve additive-array behaviour that was hardcoded in claude-cli's mergeRawConfigs is now expressed via an additivePaths option on the generic function. claude-cli passes ['execPermissions.approve']; the generic function knows nothing about the field name. claude-sdk-cli gains a proper cli-config layer (schema, consts, types, loadCliConfig) backed by ~/.claude/sdk-config.json and ./.claude/sdk-config.json. The hardcoded cliConfig.ts is deleted; model and historyReplay now come from the loaded config with Zod defaults as fallback. 9 tests cover the new schema (defaults, overrides, invalid-value fallbacks).
Creates ~/.claude/sdk-config.json with all defaults and a $schema reference if the file does not already exist. Matches the pattern from claude-cli. Also documented in --help output.
Mirrors the pattern in claude-cli/build.ts. After the esbuild step, generateJsonSchema() runs against the sdk schema and writes schema/sdk-config.schema.json at the repo root. The SCHEMA_URL in consts.ts already pointed at this path on main; now the file actually exists.
Watches CONFIG_PATH and LOCAL_CONFIG_PATH with fs.watch(). Changes are debounced 100ms then flagged as pendingReload. After each runAgent() completes the flag is checked — if set, config is reloaded and the status line model is updated before the next turn begins. The watcher variables are declared before cleanup() so SIGINT during startup (before the watch loop runs) doesn't hit the temporal dead zone.
The inline watcher state in main.ts (pendingReload flag, reloadDebounce timer, watchers array, and scheduleReload callback) was managing three distinct concerns — loading, watching, and reload signalling — in the same scope as unrelated startup logic. It was readable but the coupling would only grow. SdkConfigWatcher bundles the three concerns into one place: it loads config on construction, watches both paths, debounces the FS events, and exposes checkReload() to poll for a change after each agent turn. main.ts now only needs to know about the watcher, not how watching works.
The previous implementation set a pendingReload flag from the FS watch callback and only checked it after each runAgent() call. That meant if you edited the config file while sitting at the input prompt, nothing updated until you actually submitted a turn. The whole point of hot reload is that it happens hot. SdkConfigWatcher now takes an onChange callback in its constructor and invokes it from the debounced FS event itself. The callback runs as soon as the debounce window expires, regardless of what the agent loop is doing. main.ts passes a callback that updates the model display and logs the reload, then drops checkReload() entirely — the loop just reads watcher.config.model on each turn. Reload errors (e.g. invalid JSON, schema validation failure) are caught in #reload() and logged as a warning. The previous config is kept so the app keeps working with the last known good values.
With the previous event-driven reload, editing the config mid-turn would flip the model display to the new value while the API call was still running with the old model. The display would lie about which model was answering the current query. The model passed into runAgent is captured by value when the call starts, so the API call itself is unaffected by config changes — only the display was wrong. Fix is to track turnInProgress in main.ts and suppress display updates from the watcher callback while a turn is running. After runAgent returns, sync the display to the current config.model so any deferred change lands. The loop owns the busy flag, not the watcher — the watcher has no business knowing about agent turns. Its job is to publish config changes; the loop decides when those changes are visible.
The model the current turn is using is interesting information, and the latch we just added means the status-bar model is the *next* turn's model once a config change comes in mid-turn. Putting the model name inside the query block itself ties it to the actual turn being recorded, so you can scroll back through history and see exactly which model produced each query without checking the log file. The handler already had #model in scope (it's used for cost calculations), so it's just a matter of prepending one line to the query_summary stream: 🤖 claude-sonnet-4-5 5 system · 37 user · 37 assistant · 11 thinking The robot emoji is in the same family as the existing block emojis (💬 💭 📝 🔧 🗜 ℹ️) so visually it fits. Updated the existing 'streams the parts joined by ·' test to include the model line, plus added a new test asserting that whatever model is configured ends up in the streamed output.
Passing io:'input' to toJSONSchema() generates the schema from the user-facing input perspective rather than the parsed output. This means fields with Zod defaults are not included in required[] — which is exactly what we want for a config file schema where every field is optional and defaults fill in the gaps. The previous approach stripped required[] and additionalProperties manually in cleanSchema() using an isRoot flag. That was doing it wrong way round: generating an output schema and then patching it to look like an input schema. io:input just generates it correctly from the start. The only post-processing still needed is stripping the Number.MAX_SAFE_INTEGER maximum that Zod emits for unbounded numbers. Removed STRIP_KEYS, the isRoot parameter, and all the conditional stripping logic from cleanSchema(). The function is now a single- purpose MAX_SAFE_INTEGER cleaner. Both generated schema files updated.
a8dfe47 to
865f63a
Compare
shellicar
added a commit
that referenced
this pull request
Apr 8, 2026
CLAUDE.md current state was pointing at PR #196 (step 5b) as in-progress; the architecture refactor completed through PR #199 (step 5e) weeks ago. Config loading (#222), git delta injection (#225), and the ANSI wrap fix (#223) had also shipped without being recorded. Updated current state, added two missing recent-decisions entries, removed a duplicate closing tag. Created issue #226 for CLAUDE.md loading and added the design to cli-features.md: load order, hot reload pattern, config opt-out.
shellicar
added a commit
that referenced
this pull request
Apr 8, 2026
CLAUDE.md current state was pointing at PR #196 (step 5b) as in-progress; the architecture refactor completed through PR #199 (step 5e) weeks ago. Config loading (#222), git delta injection (#225), and the ANSI wrap fix (#223) had also shipped without being recorded. Updated current state, added two missing recent-decisions entries, removed a duplicate closing tag. Created issue #226 for CLAUDE.md loading and added the design to cli-features.md: load order, hot reload pattern, config opt-out.
shellicar
added a commit
that referenced
this pull request
Apr 9, 2026
* Add CLAUDE.md loading issue #226 and update stale harness CLAUDE.md current state was pointing at PR #196 (step 5b) as in-progress; the architecture refactor completed through PR #199 (step 5e) weeks ago. Config loading (#222), git delta injection (#225), and the ANSI wrap fix (#223) had also shipped without being recorded. Updated current state, added two missing recent-decisions entries, removed a duplicate closing tag. Created issue #226 for CLAUDE.md loading and added the design to cli-features.md: load order, hot reload pattern, config opt-out. * WIP: Add cachedReminders and CLAUDE.md loading (feature incomplete) Adds cachedReminders?: string[] to RunAgentQuery — each entry becomes a <system-reminder> block prepended to the first user message of a new conversation. Stored in history so the prefix is cached by the API on every subsequent turn. ClaudeMdLoader reads ~/.claude/CLAUDE.md, CLAUDE.md, .claude/CLAUDE.md, and CLAUDE.md.local at startup (missing files silently skipped). Content is combined under a single instruction prefix and passed as cachedReminders. Stopping here to fix a separate bug in systemReminder (injected into tool-result messages instead of only human turns) before completing this feature. Will rebase onto the fix once it lands. Closes #226 (not yet ready — resuming after systemReminder fix). * Complete CLAUDE.md loading: config flag, cachedReminders tests The WIP commit had ClaudeMdLoader, the cachedReminders SDK field, and the wiring in main.ts/runAgent.ts, but was missing two things: 1. The claudeMd.enabled config flag. Loading is on by default; setting it to false in sdk-config.json disables it entirely. Follows the same .optional().default().catch() pattern as historyReplay so invalid values silently fall back to the default rather than crashing. 2. Tests for the cachedReminders injection path in AgentRun: - injects reminder as first block when history is empty - skips injection when the conversation already has messages The second test asserts absence of a <system-reminder> block rather than string content type, because RequestBuilder converts all string content to arrays before the streamer sees it. Closes #226 * Re-inject cachedReminders after compaction The injection condition was `history.messages.length === 0`, which only covered a fresh conversation. After compaction, the history contains one message — the compaction block (assistant role) — so the condition was false and reminders were not re-injected. This is wrong. Compaction drops all content before the compaction block, including the first user message that held the cached reminders. The next human turn needs the reminders re-injected so they are present in the effective context. Fix: change the condition to check for absence of user messages rather than an empty history. After compaction only the assistant compaction block remains — no user messages — so injection correctly fires. Once the new user message (with reminders) is pushed, subsequent turns have a user message in history and injection is correctly skipped. Test added first to prove the bug: post-compaction history with only the compaction block, verifying the first user message sent to the API contains a <system-reminder> block. The test failed before the fix and passes after. * Fix Biome violations in ClaudeMdLoader files The WIP commit had two issues caught by the pre-push hook: - INSTRUCTION_PREFIX split across lines (format violation) - Braces missing on single-line if return (useBlockStatements) - Four non-null assertions in the spec (noNonNullAssertion) Replaced ! assertions with ?? '' — the tests that use the result for string operations still work correctly, and the null case would produce an empty string that fails the subsequent toContain assertions anyway. * Session log 2026-04-09 — systemReminder fix, CLAUDE.md loading * Add claudeMd to generated JSON schema * Use IFileSystem in ClaudeMdLoader: cwd/homedir from fs, read on every turn IFileSystem changed to an abstract class with cwd() added alongside homedir(), so the filesystem owns all path context. NodeFileSystem implements cwd() via process.cwd(); MemoryFileSystem takes a cwd constructor param so tests can set it alongside home. ClaudeMdLoader drops the separate cwd/home constructor params and calls fs.cwd()/fs.homedir() inside getContent(), keeping the constructor to a single IFileSystem argument. A nodeFs singleton is exported from the fs entry so callers import it directly instead of newing a NodeFileSystem. Content was read once before the loop and stored in cachedReminders, making the on-demand design pointless. It now reads inside the loop on every turn so CLAUDE.md changes are picked up without a watcher. * Fix fs imports. * Linting.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #208
What
claude-coregains a newsrc/config.tswith four generic exports:mergeRawConfigs(home, local, options?)— deep merge with optionaladditivePathsfor concatenating arrays rather than replacing themloadConfig<T>(schema, homePath, localPath, mergeOptions?)— reads home + local JSON files, merges, parses with a Zod schemacleanSchema— stripsrequired,additionalProperties, andNumber.MAX_SAFE_INTEGERmaximums from a generated JSON schemagenerateJsonSchema(schema)— calls.toJSONSchema({ target: 'draft-07' })and cleans the resultclaude-clinow imports those fromclaude-coreinstead of owning the implementations.cleanSchema.tsis deleted. TheexecPermissions.approveadditive-array behaviour is preserved by passingadditivePaths: ['execPermissions.approve']— the generic function has no knowledge of the field name.claude-sdk-cligains a proper config layer:src/cli-config/schema.ts— Zod schema coveringmodelandhistoryReplaysrc/cli-config/consts.ts— paths to~/.claude/sdk-config.jsonand./.claude/sdk-config.jsonsrc/cli-config/types.ts—ResolvedSdkConfigandHistoryReplayConfigderived from the schemasrc/cli-config/loadCliConfig.ts— thin wrapper over the coreloadConfigsrc/cli-config/initConfig.ts—--init-configflag; writes a default config with$schemato~/.claude/sdk-config.jsonif it does not already existsrc/cli-config/generateJsonSchema.ts— generatesschema/sdk-config.schema.jsonat build timesrc/cli-config/SdkConfigWatcher.ts— watches both config paths and calls anonChangecallback immediately when either file changes (100ms debounce)The hardcoded
src/cliConfig.tsis deleted.Hot reload:
main.tsconstructs anSdkConfigWatcherwith a callback that updates the model display. Changes land as soon as the debounce window expires — no need to submit a turn. While a turn is in progress the display update is deferred until the turn ends, so the model shown always matches the model the current API call is actually using. The next turn picks up the new model.Model in query block: The
🤖 <model>line is now prepended to the query summary block so you can scroll back through history and see exactly which model produced each turn without checking the log file.Tests
9 new tests in
apps/claude-sdk-cli/test/cli-config.spec.tscovering defaults, overrides, and invalid-value fallbacks.1 new test in
AgentMessageHandler.spec.tsasserting the configured model name appears in the query block stream.All existing tests pass: 239 in
claude-cli, 362 inclaude-sdk-cli.