[Fix and Refactor] Fix and enhance config slash and reload - #26
Merged
Conversation
Changing a setting from the TUI failed in a way that looked like a path problem but was a capability gap. Of 56 registered tools, none could read or write configuration, so "switch the model to X" left the model with only file_read / shell_run: it guessed at ~/.leapflow/profiles and ~/.leapflow/config/user.yaml, the workspace sandbox refused both, and the refusal wrongly suggested that approval could lift the boundary — so the user approved something unrelated and still failed. Two earlier attempts had added "do not probe <workspace>/.leapflow/config.json" to file_read's description. That forbade one path while listing the real ones, which both failed to stop the probing (the goal was unchanged, so the model tried elsewhere) and handed it fresh targets. Rather than widen allowed_roots for config paths — which would keep the model editing raw YAML, bypassing type coercion, scope checks, vault-backed secrets and hot-reload semantics — this adds the missing capability, so the sandbox stays strict and the layout becomes an implementation detail again: - config_list / config_get / config_set delegate to ConfigService, the same control plane behind `leap config` and `/config`. They take a key, never a path, so ToolExecutionContext is never consulted. - Reads are read_only/low-cost and therefore land in the CORE disclosure floor automatically (the manifest derives it); the write stays out of CORE. Both fall out of x_leapflow metadata rather than a hand-maintained list. - config_set is classified mutating_idempotent: re-setting the same value converges, which keeps it out of the uncertain-effect path that would otherwise stall a legitimate retry. - A write reloads the live session through the Context's existing reload_runtime_config_if_changed, because otherwise the value lands on disk while the in-process settings singleton keeps the old one and the model's read-back looks like a failed write. - Unknown keys return fuzzy suggestions (difflib), so typos and dropped separators are corrected in the same turn instead of falling back to probing. Writes are gated. Deep review found that x_leapflow's requires_approval only feeds capability disclosure and does not gate execution, so the tool consults an approval gate explicitly, wired from both the in-process Context and the daemon ApprovalCoordinator alongside the shell/file/gateway gates. This matters because guardrail.enabled, confirm.default_level and codegen.sandbox are all writable — an unguarded config_set would let the model switch off its own supervision. It fails closed with no gate installed, denies when the gate raises, and passes the key and metadata but never the value, so a credential cannot reach a prompt or the audit trail. Also in this change: - layout: skip the workspace-local manifest when the workspace's .leapflow *is* the LeapFlow home (typically $HOME). That collision is what left a stray workspace.yaml beside config/, profiles/ and secrets/, and is the concrete reason two .leapflow directories looked entangled. The profile-side manifest is written unconditionally, so nothing is lost. - sandbox refusals no longer imply approval can lift the boundary, and when the path is LeapFlow's own config or vault they name the config tools. The classification comes from layout.describe_path, not string matching. - file_read's description points at the config tools and no longer enumerates any LeapFlow path. 20 new tests plus two rewritten disclosure tests; each guard was verified to bite by sabotaging what it protects. Suite 1297 -> 1301 passing, ruff clean.
Symptom: after `/config llm set --model qwen3.8-max`, config_get reported qwen3.8-max and the change was live, but the status bar kept showing qwen3.7-plus. The config was never the problem. The TUI is a separate process: it seeds runtime_model_name from its own settings at startup and afterwards can only learn about a change from metadata the daemon sends back. Two paths could have carried it, and neither did: - /config returns through command_execute as a payload, and the payload already contained "model" — but the daemon-mode branch never read it. /host has exactly this kind of write-back; config was missing one. - The chat loop's "Configuration reloaded in leapd." notification is change-detection based, and a mutation force-reloads first, so the signature already matches and the notification never fires. This is why sending another message did not help either. Fixes, matched to each path: - engine_context_metadata now includes llm_model, so every status/stream path reports the model actually in use. This also covers the config_set *tool*, which changes settings mid-turn and has no payload path at all. - the daemon-mode TUI branch applies model and context length from a config mutation payload, mirroring the /host write-back. - the mutation payload carries llm_context_length alongside model: switching models usually changes it, so the 0/1M readout was stale for the same reason. An empty model is omitted rather than written, so an unset value cannot blank out what the bar already shows. Both new guards were verified to bite by removing what they cover. Suite 1301 -> 1303 passing, ruff clean.
config_set failed at runtime with "ApprovalOrchestrator.check() takes 2 positional arguments but 4 were given", so no model change through the tool ever went through. My mistake when adding the gate: I copied the call shape from file_write's gate.check(path, content, mode, meta) without checking what was actually being passed in. file_write hands set_file_write_gate a _FileWriteGate *adapter* that translates into an ActionDescriptor, whereas config wiring passes the orchestrator itself — and its check() is the shell-oriented single-argument form. The fallback made it worse: raising inside an `except TypeError` block escapes the sibling `except Exception`, so the error surfaced raw instead of failing closed. Now uses the orchestrator's native evaluate(ActionDescriptor) with ActionKind.RUNTIME_CONFIGURE, which is cleaner than adding another adapter and picks up risk assessment, policy, existing grants and the audit trail. Verified end-to-end against the real orchestrator: a change is rated HIGH and reaches the prompt, an approval writes through, a denial leaves the setting untouched. The fakes were part of the problem — they implemented the same wrong signature, so the suite stayed green while production was broken. They now implement evaluate(), and two tests drive the production ApprovalOrchestrator directly. A gate exposing only the shell-style check() is denied rather than silently allowed. Suite 1303 -> 1307 passing; reintroducing the check() call fails 7 of them.
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.
No description provided.