From 3d217b251dc2b243c5cfa1847acda76d1faf71d3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mathias=20Karst=C3=A4dt?= Date: Tue, 8 Sep 2026 23:33:10 +0200 Subject: [PATCH 1/5] feat: resolve a cmux notification setup, without pretending to deliver one MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit cmux exports `CMUX_SOCKET_PATH`, `CMUX_SURFACE_ID`, `CMUX_WORKSPACE_ID` and `CMUX` into every process it spawns, and its daemon answers JSON-RPC on the socket — so a run that finishes in a terminal nobody has looked at for ten minutes could say so. Nothing in smith knew any of it. This adds the deciding half and stops at the wire on purpose. `NotifyConfig` is the resolved answer: what is enabled, which socket, which surface, which workspace, how long to wait. `socket?` and `deliverable?` separate the two questions a caller is tempted to confuse — a socket was found, and everything needed to use one was. `CmuxClient` owns the two tiers and every rule about which one wins. The environment wins because it describes the terminal that is open now, where a config file describes one somebody had open when they wrote it. A falsey value is an absent value by one rule rather than by four: `""`, `0`, `false`, `no` and `off` each let the tier below speak, so `export CMUX_SOCKET=` and no export at all are the same statement. `CMUX` is a flag first and a path second — read as a location only when it holds a `/`, because treating `1` as a socket path would connect to a file called `1`. `Config#notify` reads the `[notify]` section and stops there. It does not reach for `ENV`, so which tier a value came from stays something you can ask; the merge is one call away. `Notify` decides that a notification should go out and what it says. It carries a `CmuxClientable` and never asks which one, so no caller branches on "am I inside cmux?" — the same line of code does nothing when there is nowhere to deliver to. A blank `body` is left out rather than sent, because an empty one renders as a gap. Extras win over the fields `notify` fills in, so a caller can send something other than a `notification` without rebuilding the payload. `deliver` swallows everything a client throws. A notification is the last thing a run does: a socket that vanished between the availability check and the write, or a cmux killed mid-run, must not become the reason a finished turn is reported as a failed one. `CmuxClient.client` returns the null client even for a complete config, and the spec that says so is written to fail loudly when a real one arrives. Until the protocol is settled — `notification.create_for_caller` against the older verbs, framing, deadline — nothing claims a notification was delivered. Co-Authored-By: Claude Opus 5 --- spec/smith/cmux_notify_spec.cr | 436 +++++++++++++++++++++++++++++++++ spec/smith/config_spec.cr | 97 ++++++++ src/smith/cmux_client.cr | 136 ++++++++++ src/smith/cmux_clientable.cr | 25 ++ src/smith/config.cr | 15 ++ src/smith/notify.cr | 114 +++++++++ src/smith/notify_config.cr | 25 ++ src/smith/null_cmux_client.cr | 20 ++ 8 files changed, 868 insertions(+) create mode 100644 spec/smith/cmux_notify_spec.cr create mode 100644 src/smith/cmux_client.cr create mode 100644 src/smith/cmux_clientable.cr create mode 100644 src/smith/notify.cr create mode 100644 src/smith/notify_config.cr create mode 100644 src/smith/null_cmux_client.cr diff --git a/spec/smith/cmux_notify_spec.cr b/spec/smith/cmux_notify_spec.cr new file mode 100644 index 0000000..5c31c01 --- /dev/null +++ b/spec/smith/cmux_notify_spec.cr @@ -0,0 +1,436 @@ +require "socket" +require "../spec_helper" +require "../../src/smith/cmux_client" +require "../../src/smith/notify" + +# Records what `Smith::Notify` hands over, so the payload rules can be asserted +# without a cmux daemon — which is the point: nothing here should depend on +# there being one. +# No `CMUX_*` at all: the shape of a shell that is not inside cmux. +private def no_cmux_env : Hash(String, String?) + {} of String => String? +end + +# A config that says everything, so "the environment wins" and "the config is +# left alone" are both observable against it. +private def filled_config : Smith::NotifyConfig + Smith::NotifyConfig.new( + enabled: true, + socket_path: "/config/cmux.sock", + surface_id: "config-surface", + workspace_id: "config-workspace" + ) +end + +private class RecordingClient < Smith::CmuxClientable + getter payloads = [] of Hash(String, JSON::Any) + property result : Bool = true + property available : Bool = true + + def notify(payload : Hash(String, JSON::Any)) : Bool + @payloads << payload + @result + end + + def available? : Bool + @available + end + + def last : Hash(String, JSON::Any) + @payloads.last + end +end + +# The client a socket implementation will eventually be: one that finds out +# something is wrong only when it tries. A notification is the last thing a +# run does, so whatever this throws must stay inside the notification. +private class ExplodingClient < Smith::CmuxClientable + def notify(payload : Hash(String, JSON::Any)) : Bool + raise Socket::ConnectError.new("Connection refused") + end + + def available? : Bool + true + end +end + +describe Smith::NotifyConfig do + it "is off, and undeliverable, until something says otherwise" do + config = Smith::NotifyConfig.new + + config.enabled.should be_false + config.socket_path.should be_nil + config.surface_id.should be_nil + config.workspace_id.should be_nil + config.socket?.should be_false + config.deliverable?.should be_false + end + + it "is deliverable only when enabled and holding a socket path" do + Smith::NotifyConfig.new(enabled: true).deliverable?.should be_false + Smith::NotifyConfig.new(socket_path: "/tmp/cmux.sock").deliverable?.should be_false + Smith::NotifyConfig.new(enabled: true, socket_path: "/tmp/cmux.sock").deliverable?.should be_true + end + + it "defaults the timeout to something a socket can live with" do + Smith::NotifyConfig.new.timeout.should eq(1.0) + end +end + +describe Smith::CmuxClient do + describe ".from_table" do + it "reads the [notify] keys" do + table = TOML.parse(<<-TOML) + enabled = true + socket_path = "/tmp/cmux.sock" + surface_id = "surface:1" + workspace_id = "workspace:1" + timeout = 2.5 + TOML + + config = Smith::CmuxClient.from_table(table) + + config.enabled.should be_true + config.socket_path.should eq("/tmp/cmux.sock") + config.surface_id.should eq("surface:1") + config.workspace_id.should eq("workspace:1") + config.timeout.should eq(2.5) + end + + it "is the off-by-default config when there is no section at all" do + config = Smith::CmuxClient.from_table(nil) + + config.enabled.should be_false + config.socket_path.should be_nil + config.deliverable?.should be_false + end + + it "reads an integer timeout as well as a float one" do + table = TOML.parse("timeout = 3") + + Smith::CmuxClient.from_table(table).timeout.should eq(3.0) + end + + it "treats a timeout that could not work as the default" do + Smith::CmuxClient.from_table(TOML.parse("timeout = 0")).timeout.should eq(1.0) + Smith::CmuxClient.from_table(TOML.parse("timeout = -1.5")).timeout.should eq(1.0) + end + + it "turns blank strings into unset, so they cannot shadow the environment" do + table = TOML.parse(<<-TOML) + enabled = true + socket_path = " " + surface_id = "" + TOML + + config = Smith::CmuxClient.from_table(table) + + config.socket_path.should be_nil + config.surface_id.should be_nil + config.socket?.should be_false + end + + it "ignores a value of the wrong type instead of raising" do + # A config file is written by a human, and `socket_path = true` is a + # typo, not a reason for smith to refuse to start. + table = TOML.parse(<<-TOML) + enabled = "yes" + socket_path = 42 + TOML + + config = Smith::CmuxClient.from_table(table) + + config.enabled.should be_false + config.socket_path.should be_nil + end + + it "trims the values it does keep" do + table = TOML.parse(<<-TOML) + socket_path = " /tmp/cmux.sock " + surface_id = " surface:1 " + TOML + + config = Smith::CmuxClient.from_table(table) + + config.socket_path.should eq("/tmp/cmux.sock") + config.surface_id.should eq("surface:1") + end + end + + describe ".resolve" do + it "leaves the config alone when the environment says nothing" do + resolved = Smith::CmuxClient.resolve(filled_config, no_cmux_env) + + resolved.enabled.should be_true + resolved.socket_path.should eq("/config/cmux.sock") + resolved.surface_id.should eq("config-surface") + resolved.workspace_id.should eq("config-workspace") + end + + it "prefers the environment: it describes the terminal that is open now" do + env = { + "CMUX_SOCKET_PATH" => "/env/cmux.sock", + "CMUX_SURFACE_ID" => "env-surface", + "CMUX_WORKSPACE_ID" => "env-workspace", + } of String => String? + + resolved = Smith::CmuxClient.resolve(filled_config, env) + + resolved.socket_path.should eq("/env/cmux.sock") + resolved.surface_id.should eq("env-surface") + resolved.workspace_id.should eq("env-workspace") + end + + it "switches notifications on from inside cmux, without a config file asking" do + off = Smith::NotifyConfig.new + + Smith::CmuxClient.resolve(off, {"CMUX" => "1"}).enabled.should be_true + end + + it "keeps them off when the config says so and cmux says nothing" do + Smith::CmuxClient.resolve(Smith::NotifyConfig.new, no_cmux_env).enabled.should be_false + end + + it "reads a falsey CMUX as cmux not being there, and leaves the config its say" do + # One rule for every variable: a falsey value is an absent value. `CMUX=0` + # in a shell that is not cmux says nothing about whether notifications + # were asked for, so the config file keeps deciding. Turning them off + # from inside cmux is `enabled = false`, which this honours — see below. + on = Smith::NotifyConfig.new(enabled: true) + off = Smith::NotifyConfig.new(enabled: false) + + %w[0 false no off].each do |value| + {"CMUX" => value, "CMUX" => value.upcase}.each do |key, spelling| + Smith::CmuxClient.resolve(on, {key => spelling}).enabled.should be_true, "#{key}=#{spelling}" + Smith::CmuxClient.resolve(off, {key => spelling}).enabled.should be_false, "#{key}=#{spelling}" + end + end + end + + it "reads an empty CMUX as unset rather than as off" do + # `export CMUX=` and no export at all are the same statement, and both + # leave the config file in charge. + on = Smith::NotifyConfig.new(enabled: true) + + Smith::CmuxClient.resolve(on, {"CMUX" => ""}).enabled.should be_true + Smith::CmuxClient.resolve(on, {"CMUX" => nil}).enabled.should be_true + end + + it "keeps the timeout: it is not something an environment describes" do + env = {"CMUX_SOCKET_PATH" => "/env/cmux.sock"} of String => String? + timed = Smith::NotifyConfig.new(timeout: 4.0) + + Smith::CmuxClient.resolve(timed, env).timeout.should eq(4.0) + end + + describe "socket path priority" do + it "takes CMUX_SOCKET_PATH over the older spellings" do + env = { + "CMUX_SOCKET_PATH" => "/canonical.sock", + "CMUX_SOCKET" => "/older.sock", + "CMUX" => "/oldest.sock", + } of String => String? + + Smith::CmuxClient.resolve(filled_config, env).socket_path.should eq("/canonical.sock") + end + + it "falls back to CMUX_SOCKET, then to CMUX when it looks like a path" do + Smith::CmuxClient.resolve(filled_config, {"CMUX_SOCKET" => "/older.sock", "CMUX" => "/oldest.sock"} of String => String?).socket_path.should eq("/older.sock") + Smith::CmuxClient.resolve(filled_config, {"CMUX" => "/tmp/cmux.sock"} of String => String?).socket_path.should eq("/tmp/cmux.sock") + end + + it "does not read a flag-shaped CMUX as a location" do + # `CMUX=1` switches notifications on. Treating `1` as a socket path + # would connect to a file called `1` in the current directory. + resolved = Smith::CmuxClient.resolve(filled_config, {"CMUX" => "1"} of String => String?) + + resolved.socket_path.should eq("/config/cmux.sock") + resolved.enabled.should be_true + end + + it "skips a falsey socket variable and keeps looking" do + env = { + "CMUX_SOCKET_PATH" => "", + "CMUX_SOCKET" => "0", + "CMUX" => "/tmp/cmux.sock", + } of String => String? + + Smith::CmuxClient.resolve(filled_config, env).socket_path.should eq("/tmp/cmux.sock") + end + + it "falls through to the config when every variable is falsey" do + env = { + "CMUX_SOCKET_PATH" => " ", + "CMUX_SOCKET" => "off", + } of String => String? + + Smith::CmuxClient.resolve(filled_config, env).socket_path.should eq("/config/cmux.sock") + end + + it "leaves a whitespace surface or workspace id unset" do + env = { + "CMUX_SURFACE_ID" => " ", + "CMUX_WORKSPACE_ID" => "", + } of String => String? + + resolved = Smith::CmuxClient.resolve(filled_config, env) + + resolved.surface_id.should eq("config-surface") + resolved.workspace_id.should eq("config-workspace") + end + end + end + + describe ".build" do + it "resolves and then builds, in one step" do + client = Smith::CmuxClient.build( + Smith::NotifyConfig.new(enabled: true), + {"CMUX_SOCKET_PATH" => "/tmp/cmux.sock"} of String => String? + ) + + client.should be_a(Smith::CmuxClientable) + end + end + + describe ".client" do + it "is null when notifications are off" do + Smith::CmuxClient.client(Smith::NotifyConfig.new(socket_path: "/tmp/cmux.sock")).should be_a(Smith::NullCmuxClient) + end + + it "is null when there is no socket to talk to" do + Smith::CmuxClient.client(Smith::NotifyConfig.new(enabled: true)).should be_a(Smith::NullCmuxClient) + end + + it "is null even when everything is in place, until the wire exists" do + # The seam this PR builds stops here on purpose. When a real client + # arrives, this is the expectation that changes — and it should change + # loudly, not by a spec quietly going green. + config = Smith::NotifyConfig.new(enabled: true, socket_path: "/tmp/cmux.sock") + + Smith::CmuxClient.client(config).should be_a(Smith::NullCmuxClient) + end + end +end + +describe Smith::NullCmuxClient do + it "accepts nothing and delivers nothing" do + client = Smith::NullCmuxClient.new + + client.available?.should be_false + client.notify(Hash(String, JSON::Any).new).should be_false + end +end + +describe Smith::Notify do + it "names what kind of message it is sending" do + client = RecordingClient.new + Smith::Notify.new(client).notify("Build done") + + client.last["type"].as_s.should eq("notification") + end + + it "always sends a title, even an empty one" do + client = RecordingClient.new + Smith::Notify.new(client).notify("") + + client.last["title"].as_s.should eq("") + end + + it "sends subtitle and body when they say something" do + client = RecordingClient.new + Smith::Notify.new(client).notify("Build done", subtitle: "2 failed", body: "spec/smith/notify_spec.cr") + + client.last["subtitle"].as_s.should eq("2 failed") + client.last["body"].as_s.should eq("spec/smith/notify_spec.cr") + end + + it "leaves an absent subtitle or body out of the payload entirely" do + client = RecordingClient.new + Smith::Notify.new(client).notify("Build done") + + client.last.has_key?("subtitle").should be_false + client.last.has_key?("body").should be_false + end + + it "leaves a blank body out, because an empty one renders as a gap" do + client = RecordingClient.new + Smith::Notify.new(client).notify("Build done", subtitle: " ", body: "") + + client.last.has_key?("subtitle").should be_false + client.last.has_key?("body").should be_false + end + + it "trims what it does send" do + client = RecordingClient.new + Smith::Notify.new(client).notify("Build done", body: " all green ") + + client.last["body"].as_s.should eq("all green") + end + + it "carries extra fields into the payload" do + client = RecordingClient.new + Smith::Notify.new(client).notify("Build done", priority: "high", count: 3, ratio: 0.5, urgent: true, missing: nil) + + client.last["priority"].as_s.should eq("high") + client.last["count"].as_i.should eq(3) + client.last["ratio"].as_f.should eq(0.5) + client.last["urgent"].as_bool.should be_true + client.last["missing"].raw.should be_nil + end + + it "carries nested extras, widening the numbers JSON wants" do + client = RecordingClient.new + Smith::Notify.new(client).notify("Build done", tags: ["ci", "main"], detail: {"file" => "notify.cr", "line" => 12}) + + client.last["tags"].as_a.map(&.as_s).should eq(["ci", "main"]) + client.last["detail"]["file"].as_s.should eq("notify.cr") + client.last["detail"]["line"].as_i.should eq(12) + end + + it "lets an extra win over the field notify would have filled in" do + # The caller knows better than the default here: a `type` of something + # other than "notification" is a deliberate choice, not a collision. + client = RecordingClient.new + Smith::Notify.new(client).notify("Build done", type: "status") + + client.last["type"].as_s.should eq("status") + client.last["title"].as_s.should eq("Build done") + end + + it "reports what the client reported" do + client = RecordingClient.new + Smith::Notify.new(client).notify("Build done").should be_true + + client.result = false + Smith::Notify.new(client).notify("Build done").should be_false + end + + it "reports unavailable, and sends nothing, through a null client" do + notify = Smith::Notify.new(Smith::NullCmuxClient.new) + + notify.enabled?.should be_false + notify.notify("Build done").should be_false + end + + it "reports available when the client has somewhere to deliver to" do + client = RecordingClient.new + Smith::Notify.new(client).enabled?.should be_true + + client.available = false + Smith::Notify.new(client).enabled?.should be_false + end + + it "swallows a client that throws, because a notification must not end a run" do + # The socket can vanish between the availability check and the write, and + # cmux can be killed mid-run. Both are ordinary, and neither is worth the + # turn that was just completed. + Smith::Notify.new(ExplodingClient.new).notify("Build done").should be_false + end + + it "serialises to the JSON a daemon would receive" do + client = RecordingClient.new + Smith::Notify.new(client).notify("Build done", subtitle: "2 failed") + + client.last.to_json.should eq(%({"type":"notification","title":"Build done","subtitle":"2 failed"})) + end +end diff --git a/spec/smith/config_spec.cr b/spec/smith/config_spec.cr index cbb9e45..9c032bc 100644 --- a/spec/smith/config_spec.cr +++ b/spec/smith/config_spec.cr @@ -839,3 +839,100 @@ describe "session settings" do end end end + +# Only the config file tier: merging the `CMUX_*` environment into this is +# `Smith::CmuxClient.resolve`, covered in cmux_notify_spec.cr. Keeping the two +# apart here is what makes it possible to say which tier a value came from. +describe "notify settings" do + it "is off and empty until a [notify] section says otherwise" do + with_sandbox do |temp_dir, _home| + settings = Smith::Config.load(make_project(temp_dir)).notify + + settings.enabled.should be_false + settings.socket_path.should be_nil + settings.surface_id.should be_nil + settings.workspace_id.should be_nil + settings.timeout.should eq(1.0) + end + end + + it "reads the section" do + with_sandbox do |temp_dir, _home| + project = make_project(temp_dir, <<-TOML) + [notify] + enabled = true + socket_path = "/tmp/cmux.sock" + surface_id = "surface:1" + workspace_id = "workspace:1" + timeout = 2.5 + TOML + + settings = Smith::Config.load(project).notify + + settings.enabled.should be_true + settings.socket_path.should eq("/tmp/cmux.sock") + settings.surface_id.should eq("surface:1") + settings.workspace_id.should eq("workspace:1") + settings.timeout.should eq(2.5) + settings.socket?.should be_true + settings.deliverable?.should be_true + end + end + + it "lets the project config override the global one, key by key" do + with_sandbox do |temp_dir, home_dir| + File.write(File.join(home_dir, "config.toml"), <<-TOML) + [notify] + enabled = true + socket_path = "/global/cmux.sock" + surface_id = "global-surface" + TOML + + project = make_project(temp_dir, <<-TOML) + [notify] + surface_id = "project-surface" + TOML + + settings = Smith::Config.load(project).notify + + settings.enabled.should be_true + settings.socket_path.should eq("/global/cmux.sock") + settings.surface_id.should eq("project-surface") + end + end + + it "turns a blank setting into unset rather than into an empty value" do + # An empty `socket_path` is a key someone left behind, not a location. Read + # as a location it would shadow the environment's with nothing. + with_sandbox do |temp_dir, _home| + project = make_project(temp_dir, <<-TOML) + [notify] + socket_path = " " + surface_id = "" + TOML + + settings = Smith::Config.load(project).notify + + settings.socket_path.should be_nil + settings.surface_id.should be_nil + settings.socket?.should be_false + end + end + + it "ignores values of the wrong type instead of refusing to start" do + with_sandbox do |temp_dir, _home| + project = make_project(temp_dir, <<-TOML) + [notify] + enabled = "yes" + socket_path = 42 + timeout = "soon" + TOML + + settings = Smith::Config.load(project).notify + + settings.enabled.should be_false + settings.socket_path.should be_nil + settings.timeout.should eq(1.0) + end + end +end diff --git a/src/smith/cmux_client.cr b/src/smith/cmux_client.cr new file mode 100644 index 0000000..12e7065 --- /dev/null +++ b/src/smith/cmux_client.cr @@ -0,0 +1,136 @@ +require "toml" +require "./cmux_clientable" +require "./null_cmux_client" +require "./notify_config" + +module Smith + # Turns the two places a cmux notification setup can be described — the + # `[notify]` section of config.toml and the `CMUX_*` environment the cmux + # terminal exports into every process it spawns — into one `NotifyConfig`, + # and that into something `Smith::Notify` can talk to. + # + # This is the only place that knows those names. `Smith::Notify` sees a + # resolved config and a `CmuxClientable`; neither knows there is an + # environment, and neither reaches for one. + module CmuxClient + # In priority order. cmux itself documents `CMUX_SOCKET_PATH`; the other + # two are the older spellings still found in the wild, so they are checked + # rather than argued with. + SOCKET_PATH_KEYS = {"CMUX_SOCKET_PATH", "CMUX_SOCKET", "CMUX"} + + # Values an environment variable can hold that mean "not set" rather than + # "set to this". A shell that exports `CMUX_SOCKET=` is saying the same + # thing as one that never exported it, and `CMUX=0` is how a program turns + # a flag off without unsetting it — in both cases the next tier down gets + # its say instead. + FALSEY = {"", "0", "false", "no", "off"} + + DEFAULT_TIMEOUT = 1.0 + + # The config tier, before the environment has had a say. Blank strings + # become nil, so `socket_path = ""` in a config file is the same as the key + # not being there — otherwise the empty value would shadow the environment + # with nothing. + def self.from_table(table : Hash(String, TOML::Any)? = nil) : NotifyConfig + enabled = setting(table, "enabled").try(&.as_bool?) + timeout = float_setting(table, "timeout") + + NotifyConfig.new( + enabled: enabled.nil? ? false : enabled, + socket_path: normalize(setting(table, "socket_path").try(&.as_s?)), + surface_id: normalize(setting(table, "surface_id").try(&.as_s?)), + workspace_id: normalize(setting(table, "workspace_id").try(&.as_s?)), + timeout: timeout.nil? || timeout <= 0 ? DEFAULT_TIMEOUT : timeout + ) + end + + # Config plus environment, environment winning. Inside cmux the variables + # describe the terminal that is running right now — this surface, this + # workspace, this socket — so they are the more accurate answer than + # anything a config file could have been written with. + # + # `env` is a parameter rather than `ENV` so the resolution is testable + # without touching the process environment. + def self.resolve(config : NotifyConfig, env : Hash(String, String?) = ENV) : NotifyConfig + cmux = truthy(env, "CMUX") + + NotifyConfig.new( + # `CMUX` being *truthy* is cmux announcing "you are inside me", which + # is the same statement as `enabled = true` — and the one made about + # the terminal actually in use. A falsey `CMUX` is one rule for every + # variable: an absent value. It says nothing about notifications, so + # the config file keeps deciding; turning them off from inside cmux is + # `enabled = false`, which this honours. + enabled: cmux.nil? ? config.enabled : true, + socket_path: socket_path(env, config.socket_path), + surface_id: presence(env, "CMUX_SURFACE_ID") || config.surface_id, + workspace_id: presence(env, "CMUX_WORKSPACE_ID") || config.workspace_id, + timeout: config.timeout + ) + end + + # Resolve, then build. The one call a caller that is not itself resolving + # anything needs. + def self.build(config : NotifyConfig, env : Hash(String, String?) = ENV) : CmuxClientable + client(resolve(config, env)) + end + + # The client for an already-resolved config. Kept separate from `resolve` + # because resolving is pure: a diagnostic can compute the effective config + # without anything being connected. + # + # Null whenever delivering is impossible, so the caller never has to ask + # first. + def self.client(config : NotifyConfig) : CmuxClientable + return NullCmuxClient.new unless config.deliverable? + + # TODO: open the unix socket at `config.socket_path` and speak to it + # (#120). Until then every config resolves to null — the plumbing is + # here, the wire is not, and nothing silently claims a notification was + # delivered. + NullCmuxClient.new + end + + private def self.socket_path(env : Hash(String, String?), configured : String?) : String? + SOCKET_PATH_KEYS.each do |key| + value = truthy(env, key) + next if value.nil? + # `CMUX` is a flag first and a path second: only when it holds + # something that looks like a location is it read as one. + next if key == "CMUX" && !value.includes?("/") + return value + end + + configured + end + + private def self.setting(table : Hash(String, TOML::Any)?, key : String) : TOML::Any? + table.try(&.[key]?) + end + + # TOML writes `timeout = 2` as an integer and `timeout = 0.5` as a float, + # and both are the same statement — `as_f?` reads either. + private def self.float_setting(table : Hash(String, TOML::Any)?, key : String) : Float64? + setting(table, key).try(&.as_f?) + end + + # A set variable that says something. Whitespace-only and the usual + # spellings of "off" come back as nil, which is what lets the tier below + # speak. + private def self.truthy(env : Hash(String, String?), key : String) : String? + value = normalize(env[key]?) + return nil if value.nil? + FALSEY.includes?(value.downcase) ? nil : value + end + + private def self.presence(env : Hash(String, String?), key : String) : String? + normalize(env[key]?) + end + + private def self.normalize(value : String?) : String? + return nil if value.nil? + stripped = value.strip + stripped.empty? ? nil : stripped + end + end +end diff --git a/src/smith/cmux_clientable.cr b/src/smith/cmux_clientable.cr new file mode 100644 index 0000000..7c98c58 --- /dev/null +++ b/src/smith/cmux_clientable.cr @@ -0,0 +1,25 @@ +require "json" + +module Smith + # The seam `Smith::Notify` talks to: something that can hand a cmux + # notification payload to a cmux daemon. + # + # Deliberately the whole surface. It exposes no socket, no path and no + # protocol, so neither the caller nor the specs need to know which one the + # real implementation happens to speak — and the null one can keep pretending + # there is a daemon at all. + abstract class CmuxClientable + # Hand a ready-made notification payload to cmux. + # + # `payload` is what goes over the wire, keys and all — implementations only + # transport it. Returns true when cmux accepted it, false when it did not + # or when there was nothing to accept. A failure here is never an + # exception: notification delivery must not be able to take a run down. + abstract def notify(payload : Hash(String, JSON::Any)) : Bool + + # True when there is somewhere to deliver to. Used for diagnostics, never + # as a precondition — `notify` on a client that answers false is a no-op, + # not an error. + abstract def available? : Bool + end +end diff --git a/src/smith/config.cr b/src/smith/config.cr index 825e4a4..43dc523 100644 --- a/src/smith/config.cr +++ b/src/smith/config.cr @@ -9,6 +9,8 @@ require "./mentions" require "./sandbox" require "./media" require "./pricing" +require "./notify_config" +require "./cmux_client" module Smith # Resolved configuration, merged from (lowest to highest priority): @@ -627,6 +629,19 @@ module Smith ) end + # The `[notify]` section, and only that: what the config file says about + # cmux desktop notifications. The `CMUX_*` environment cmux exports into + # the processes it spawns is the other half, and merging the two is + # `Smith::CmuxClient.resolve`'s job — this deliberately does not reach for + # `ENV`, so a config file and a terminal can be reasoned about separately. + # + # The result is a `NotifyConfig` rather than a nested struct here because + # `CmuxClient` already owns every rule about which value wins; keeping two + # shapes would mean keeping two sets. + def notify : NotifyConfig + CmuxClient.from_table(lookup("notify").try(&.as_h?)) + end + # Consumed by Subagents::Supervisor via CLI#build_agent. max_children = 0 # switches subagents off entirely — the agent tool is then not registered. def subagents : SubagentSettings diff --git a/src/smith/notify.cr b/src/smith/notify.cr new file mode 100644 index 0000000..2e0b4f1 --- /dev/null +++ b/src/smith/notify.cr @@ -0,0 +1,114 @@ +require "json" +require "./cmux_clientable" +require "./null_cmux_client" + +module Smith + # Decides that a notification should go out, and what it says. Nothing + # more: which socket carries it, which verb cmux wants and whether the + # daemon is even running are the client's business, and this class never + # asks. + # + # It is built once per run from a `CmuxClientable` — the real one when cmux + # is reachable, `NullCmuxClient` in every other case — so no caller has to + # branch on "am I inside cmux?". The same line of code works either way and + # just does nothing when there is nowhere to deliver to. + class Notify + # What a payload field may hold. Recursive so extras can carry a small + # nested value, and narrow enough that a caller passing something + # unserialisable finds out at compile time rather than at the end of a + # long run. + alias Field = String | Int32 | Int64 | Float64 | Bool | Nil | Array(Field) | Hash(String, Field) + + # The discriminant of what kind of message this is. cmux also accepts + # status lines and progress updates; naming the kind here is what lets one + # client speak to all of them later without this class growing a second + # method per message type. + TYPE = "notification" + + def initialize(@client : CmuxClientable) + end + + # True when there is somewhere to deliver to. Purely informational — the + # caller does not need it to call `notify`, and should not skip on it: a + # no-op is exactly what "not inside cmux" means here. + def enabled? : Bool + @client.available? + end + + # Send a notification. Returns true only when cmux took it; false covers + # "nowhere to send", "cmux refused" and "something threw", which the + # caller has no way to tell apart and no reason to. + # + # `title` is always sent, even empty — it is the one field a notification + # is identified by. `subtitle` and `body` are sent only when they say + # something, because an empty body renders as a gap rather than as nothing. + # + # `extra` lands in the payload as given and wins over `type`, `subtitle` + # and `body`: a caller that wants a different discriminant, or wants a + # blank body kept, says so here instead of rebuilding the payload. `title` + # cannot be overridden this way — it is a declared parameter, and Crystal + # refuses a named argument that repeats one. + # + # The splat is untyped and the `Field` restriction is enforced by + # `to_any` instead. Restricting it here would be the more honest + # signature, but a typed double splat on a method that also has defaulted + # arguments cannot be called without naming one — Crystal 1.21 rejects + # `notify("x")` — and "notify with nothing but a title" is the common case. + def notify(title : String, subtitle : String? = nil, body : String? = nil, **extra) : Bool + payload = Hash(String, JSON::Any).new + payload["type"] = JSON::Any.new(TYPE) + payload["title"] = JSON::Any.new(title) + put(payload, "subtitle", subtitle) + put(payload, "body", body) + + extra.each do |key, value| + payload[key.to_s] = to_any(value) + end + + deliver(payload) + end + + # A client that is not supposed to throw says so in its contract, and a + # real one that talks to a socket will anyway — the socket can vanish + # between the check and the write, and cmux can be killed mid-run. Either + # way this is the last thing a finishing run does, and "the notification + # failed" must never be the reason a run fails. + private def deliver(payload : Hash(String, JSON::Any)) : Bool + @client.notify(payload) + rescue ex : Exception + false + end + + private def put(payload : Hash(String, JSON::Any), key : String, value : String?) : Nil + return if value.nil? + stripped = value.strip + return if stripped.empty? + + payload[key] = JSON::Any.new(stripped) + end + + # `Field` down to something `JSON::Any` can hold. Written out rather than + # round-tripped through `to_json` and `JSON.parse` because the recursion + # has to happen here: an `Array(Field)` or a `Hash(String, Field)` is not + # itself a JSON type, only its leaves are, and a leaf arrives as an `Int32` + # where `JSON::Any` wants an `Int64`. A value the alias does not admit is + # refused where it is passed, which is the point of having the alias. + private def to_any(value : Field) : JSON::Any + case value + when Array + JSON::Any.new(value.map { |item| to_any(item) }) + when Hash + JSON::Any.new(value.to_h { |key, item| {key, to_any(item)} }) + when Int32 + JSON::Any.new(value.to_i64) + else + # Nil, Bool, Int64, Float64 and String: the scalar half of + # `JSON::Any::Type`, so they go over as they are. This `else` is a + # branch rather than the fifth `when` because Crystal cannot prove a + # recursive alias exhaustive, and an unreachable `raise` would be the + # less honest of the two ways to say so. + JSON::Any.new(value) + end + end + end +end diff --git a/src/smith/notify_config.cr b/src/smith/notify_config.cr new file mode 100644 index 0000000..e7b6abf --- /dev/null +++ b/src/smith/notify_config.cr @@ -0,0 +1,25 @@ +module Smith + # Resolved configuration for cmux desktop notifications. Pure data: it holds + # the effective values after config and environment have been merged, and + # knows nothing about sockets or the wire protocol. + # + # Blank strings are normalised to `nil` so callers can treat "unset" and + # "explicitly empty" the same way. + record NotifyConfig, + enabled : Bool = false, + socket_path : String? = nil, + surface_id : String? = nil, + workspace_id : String? = nil, + timeout : Float64 = 1.0 do + # True when a socket path was resolved. Without one there is no cmux + # daemon to talk to, so notifications degrade to a no-op. + def socket? : Bool + !@socket_path.nil? + end + + # True when everything needed to actually deliver is present. + def deliverable? : Bool + @enabled && socket? + end + end +end diff --git a/src/smith/null_cmux_client.cr b/src/smith/null_cmux_client.cr new file mode 100644 index 0000000..a305573 --- /dev/null +++ b/src/smith/null_cmux_client.cr @@ -0,0 +1,20 @@ +require "./cmux_clientable" + +module Smith + # The client for every case where there is nothing to deliver to: cmux + # notifications are off, no socket was resolved, or the daemon is not there. + # + # It exists so the caller does not branch. `Smith::Notify` builds a client + # and calls `notify` either way, and the difference between "cmux is not + # running" and "cmux is running" never reaches the code that decides a run + # is finished and the human should hear about it. + class NullCmuxClient < CmuxClientable + def notify(payload : Hash(String, JSON::Any)) : Bool + false + end + + def available? : Bool + false + end + end +end From e81d4e03f90e2a2c3397c37b52bae52052848ec1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mathias=20Karst=C3=A4dt?= Date: Wed, 9 Sep 2026 01:33:43 +0200 Subject: [PATCH 2/5] fix: read being inside cmux off the socket it exports, not a flag it never sets MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The resolution switched notifications on by reading `CMUX` truthy. cmux does not export `CMUX` — checked against its own environment and its own docs, which name `CMUX_SOCKET_PATH`, `CMUX_SURFACE_ID`, `CMUX_WORKSPACE_ID`, `CMUX_TAB_ID` and `CMUX_PANEL_ID`. So the branch that decided "you are inside cmux" never fired, and notifications were unreachable without a config file that asked for them by hand. What does say it is the socket path: cmux exports one into every process it spawns, so its presence is the announcement that this session is running inside a cmux terminal, which is the situation a completion notification exists for. The default is now on inside cmux and off everywhere else, so a plain terminal run is unchanged by this feature and a cmux one is not. Which makes `enabled` tri-state, and that is the part the shape had to change for. `false` is somebody turning notifications off and must win over the terminal; `nil` is nobody having said, which is the only answer the terminal may overrule. A config file that never mentions `[notify]` was being read as one that refused it. `||` was not enough to tell the two apart — it collapses `false` into `nil` and the specs caught an explicit `enabled = false` being switched back on by the socket beside it. Two further corrections, both from reading the environment rather than guessing at it: - `CMUX_SOCKET` is exported *empty* alongside a populated `CMUX_SOCKET_PATH`. A resolution reading the first variable that is set rather than the first value that says something finds no socket at all — and now concludes the session is not inside cmux, since the socket is what says so. - `CMUX_TAB_ID`/`CMUX_PANEL_ID` carry the same two ids as `CMUX_WORKSPACE_ID`/`CMUX_SURFACE_ID`, verified equal in the same shell. Both pairs are read, so neither spelling is assumed. `resolve` and `build` also default to a snapshot of the environment rather than to `ENV` itself, which is not a `Hash`. Nothing called them yet, so a default argument that could not typecheck was never compiled against one; the next commit does, and this makes it able to. Co-Authored-By: Claude Opus 5 --- spec/smith/cmux_notify_spec.cr | 126 +++++++++++++++++++++++++-------- spec/smith/config_spec.cr | 6 +- src/smith/cmux_client.cr | 124 +++++++++++++++++++++++++------- src/smith/notify_config.cr | 17 ++++- 4 files changed, 213 insertions(+), 60 deletions(-) diff --git a/spec/smith/cmux_notify_spec.cr b/spec/smith/cmux_notify_spec.cr index 5c31c01..fe3eaab 100644 --- a/spec/smith/cmux_notify_spec.cr +++ b/spec/smith/cmux_notify_spec.cr @@ -55,10 +55,14 @@ private class ExplodingClient < Smith::CmuxClientable end describe Smith::NotifyConfig do - it "is off, and undeliverable, until something says otherwise" do + it "says nothing at all, and delivers nothing, until something does" do config = Smith::NotifyConfig.new - config.enabled.should be_false + # `enabled` is tri-state on purpose: nil is "nobody said", which is the + # answer a terminal is allowed to overrule. false would be somebody + # turning notifications off, and no environment may overrule that. + config.enabled.should be_nil + config.enabled?.should be_false config.socket_path.should be_nil config.surface_id.should be_nil config.workspace_id.should be_nil @@ -72,6 +76,10 @@ describe Smith::NotifyConfig do Smith::NotifyConfig.new(enabled: true, socket_path: "/tmp/cmux.sock").deliverable?.should be_true end + it "is not deliverable when explicitly turned off, however much else is there" do + Smith::NotifyConfig.new(enabled: false, socket_path: "/tmp/cmux.sock").deliverable?.should be_false + end + it "defaults the timeout to something a socket can live with" do Smith::NotifyConfig.new.timeout.should eq(1.0) end @@ -97,10 +105,10 @@ describe Smith::CmuxClient do config.timeout.should eq(2.5) end - it "is the off-by-default config when there is no section at all" do + it "says nothing when there is no section at all" do config = Smith::CmuxClient.from_table(nil) - config.enabled.should be_false + config.enabled.should be_nil config.socket_path.should be_nil config.deliverable?.should be_false end @@ -140,7 +148,9 @@ describe Smith::CmuxClient do config = Smith::CmuxClient.from_table(table) - config.enabled.should be_false + # Not false: an unreadable `enabled` is nobody having said, so the + # terminal keeps its say rather than a typo switching notifications off. + config.enabled.should be_nil config.socket_path.should be_nil end @@ -161,7 +171,7 @@ describe Smith::CmuxClient do it "leaves the config alone when the environment says nothing" do resolved = Smith::CmuxClient.resolve(filled_config, no_cmux_env) - resolved.enabled.should be_true + resolved.enabled?.should be_true resolved.socket_path.should eq("/config/cmux.sock") resolved.surface_id.should eq("config-surface") resolved.workspace_id.should eq("config-workspace") @@ -181,39 +191,64 @@ describe Smith::CmuxClient do resolved.workspace_id.should eq("env-workspace") end - it "switches notifications on from inside cmux, without a config file asking" do - off = Smith::NotifyConfig.new + it "switches notifications on inside cmux, without a config file asking" do + # cmux exports a socket path into every process it spawns; that is how + # smith knows this session is running inside one. There is no `CMUX=1` + # flag to read, and the terminal's own environment is checked below. + silent = Smith::NotifyConfig.new - Smith::CmuxClient.resolve(off, {"CMUX" => "1"}).enabled.should be_true + Smith::CmuxClient.resolve(silent, {"CMUX_SOCKET_PATH" => "/cmux.sock"}).enabled?.should be_true end - it "keeps them off when the config says so and cmux says nothing" do - Smith::CmuxClient.resolve(Smith::NotifyConfig.new, no_cmux_env).enabled.should be_false + it "leaves notifications off in a shell that is not cmux" do + # The default has to be "nothing happens" — a plain terminal session + # must not be changed by this feature, and must not spend a turn + # looking for a socket that was never there. + Smith::CmuxClient.resolve(Smith::NotifyConfig.new, no_cmux_env).enabled?.should be_false end - it "reads a falsey CMUX as cmux not being there, and leaves the config its say" do - # One rule for every variable: a falsey value is an absent value. `CMUX=0` - # in a shell that is not cmux says nothing about whether notifications - # were asked for, so the config file keeps deciding. Turning them off - # from inside cmux is `enabled = false`, which this honours — see below. - on = Smith::NotifyConfig.new(enabled: true) + it "honours an explicit enabled = false over the terminal" do + # Somebody turned them off. Being inside cmux is not a reason to + # overrule that, which is why `false` is not folded into "unset". off = Smith::NotifyConfig.new(enabled: false) + resolved = Smith::CmuxClient.resolve(off, {"CMUX_SOCKET_PATH" => "/cmux.sock"}) + + resolved.enabled?.should be_false + resolved.deliverable?.should be_false + end + + it "does not read a socket path from a config file as being inside cmux" do + # Naming a location is not the same statement as the terminal making + # itself known: a config file can point at a socket for a session that + # is not running inside cmux at all. Left to `enabled` to ask for. + configured = Smith::NotifyConfig.new(socket_path: "/config/cmux.sock") + + Smith::CmuxClient.resolve(configured, no_cmux_env).enabled?.should be_false + end + + it "reads a falsey socket variable as cmux not being there" do + # One rule for every variable: a falsey value is an absent value, so it + # cannot switch notifications on either. This is not a hypothetical — + # cmux exports `CMUX_SOCKET=` empty alongside a populated + # `CMUX_SOCKET_PATH` in the same shell. %w[0 false no off].each do |value| - {"CMUX" => value, "CMUX" => value.upcase}.each do |key, spelling| - Smith::CmuxClient.resolve(on, {key => spelling}).enabled.should be_true, "#{key}=#{spelling}" - Smith::CmuxClient.resolve(off, {key => spelling}).enabled.should be_false, "#{key}=#{spelling}" + {"CMUX_SOCKET_PATH" => value, "CMUX_SOCKET_PATH" => value.upcase}.each do |key, spelling| + env = {key => spelling} of String => String? + Smith::CmuxClient.resolve(Smith::NotifyConfig.new, env).enabled?.should be_false, "#{key}=#{spelling}" end end end - it "reads an empty CMUX as unset rather than as off" do - # `export CMUX=` and no export at all are the same statement, and both - # leave the config file in charge. - on = Smith::NotifyConfig.new(enabled: true) + it "reads an empty socket variable as unset rather than as a location" do + # `export CMUX_SOCKET=` and no export at all are the same statement. + # Read as a location it would shadow the real one with nothing. + env = {"CMUX_SOCKET" => ""} of String => String? - Smith::CmuxClient.resolve(on, {"CMUX" => ""}).enabled.should be_true - Smith::CmuxClient.resolve(on, {"CMUX" => nil}).enabled.should be_true + resolved = Smith::CmuxClient.resolve(filled_config, env) + + resolved.socket_path.should eq("/config/cmux.sock") + resolved.deliverable?.should be_true end it "keeps the timeout: it is not something an environment describes" do @@ -240,12 +275,15 @@ describe Smith::CmuxClient do end it "does not read a flag-shaped CMUX as a location" do - # `CMUX=1` switches notifications on. Treating `1` as a socket path + # `CMUX=1` is the shape of a flag, not of a path. Treating `1` as one # would connect to a file called `1` in the current directory. - resolved = Smith::CmuxClient.resolve(filled_config, {"CMUX" => "1"} of String => String?) + silent = Smith::NotifyConfig.new + resolved = Smith::CmuxClient.resolve(silent, {"CMUX" => "1"} of String => String?) - resolved.socket_path.should eq("/config/cmux.sock") - resolved.enabled.should be_true + resolved.socket_path.should be_nil + # …and a flag is not a socket, so it cannot switch notifications on + # either: nothing in this environment says "you are inside cmux". + resolved.enabled?.should be_false end it "skips a falsey socket variable and keeps looking" do @@ -278,6 +316,34 @@ describe Smith::CmuxClient do resolved.surface_id.should eq("config-surface") resolved.workspace_id.should eq("config-workspace") end + + it "takes a documented spelling when the exported one is missing" do + # cmux documents `CMUX_TAB_ID` and `CMUX_PANEL_ID` and exports + # `CMUX_WORKSPACE_ID`/`CMUX_SURFACE_ID` carrying the same two values. + # Which pair a build offers is not something smith can ask about, so + # neither is assumed. + silent = Smith::NotifyConfig.new + env = {"CMUX_TAB_ID" => "tab-1", "CMUX_PANEL_ID" => "panel-1"} of String => String? + + resolved = Smith::CmuxClient.resolve(silent, env) + + resolved.workspace_id.should eq("tab-1") + resolved.surface_id.should eq("panel-1") + end + + it "prefers the workspace and surface ids it was documented with" do + env = { + "CMUX_WORKSPACE_ID" => "workspace-1", + "CMUX_TAB_ID" => "tab-1", + "CMUX_SURFACE_ID" => "surface-1", + "CMUX_PANEL_ID" => "panel-1", + } of String => String? + + resolved = Smith::CmuxClient.resolve(Smith::NotifyConfig.new, env) + + resolved.workspace_id.should eq("workspace-1") + resolved.surface_id.should eq("surface-1") + end end end diff --git a/spec/smith/config_spec.cr b/spec/smith/config_spec.cr index 9c032bc..32ae8e4 100644 --- a/spec/smith/config_spec.cr +++ b/spec/smith/config_spec.cr @@ -848,7 +848,9 @@ describe "notify settings" do with_sandbox do |temp_dir, _home| settings = Smith::Config.load(make_project(temp_dir)).notify - settings.enabled.should be_false + # nil, not false: nobody asked either way, and that is the answer + # `CmuxClient.resolve` lets the terminal overrule. + settings.enabled.should be_nil settings.socket_path.should be_nil settings.surface_id.should be_nil settings.workspace_id.should be_nil @@ -930,7 +932,7 @@ describe "notify settings" do settings = Smith::Config.load(project).notify - settings.enabled.should be_false + settings.enabled.should be_nil settings.socket_path.should be_nil settings.timeout.should eq(1.0) end diff --git a/src/smith/cmux_client.cr b/src/smith/cmux_client.cr index 12e7065..976594a 100644 --- a/src/smith/cmux_client.cr +++ b/src/smith/cmux_client.cr @@ -13,16 +13,39 @@ module Smith # resolved config and a `CmuxClientable`; neither knows there is an # environment, and neither reaches for one. module CmuxClient - # In priority order. cmux itself documents `CMUX_SOCKET_PATH`; the other - # two are the older spellings still found in the wild, so they are checked - # rather than argued with. + # In priority order. `CMUX_SOCKET_PATH` is the one cmux documents and the + # one its environment actually carries. + # + # The other two are kept because they cost nothing to check and because a + # spelling smith refused to read is a notification that silently does not + # arrive: `CMUX_SOCKET` is exported alongside the documented name — empty, + # in the environment this was written against, which is exactly why a + # resolution has to read the first value that says something rather than + # the first variable that is set — and `CMUX` is the spelling #120 named + # and the one a wrapper script is most likely to set itself. SOCKET_PATH_KEYS = {"CMUX_SOCKET_PATH", "CMUX_SOCKET", "CMUX"} + # Two pairs of names for the same two values. cmux's own CLI and docs speak + # of tabs and panels, and the environment this was written against exports + # `CMUX_WORKSPACE_ID` and `CMUX_SURFACE_ID` carrying the same two ids its + # `CMUX_TAB_ID` and `CMUX_PANEL_ID` do — both halves verified, not assumed. + # + # Reading both costs nothing, and which pair a build exports is not + # something smith can ask about. The workspace and surface names come + # first: they are the ones observed, and the tab and panel names are the + # ones a build might stop exporting. + WORKSPACE_ID_KEYS = {"CMUX_WORKSPACE_ID", "CMUX_TAB_ID"} + SURFACE_ID_KEYS = {"CMUX_SURFACE_ID", "CMUX_PANEL_ID"} + # Values an environment variable can hold that mean "not set" rather than - # "set to this". A shell that exports `CMUX_SOCKET=` is saying the same - # thing as one that never exported it, and `CMUX=0` is how a program turns - # a flag off without unsetting it — in both cases the next tier down gets - # its say instead. + # "set to this": a shell that exports one of these is saying the same thing + # as one that never exported it, so the next tier down gets its say. + # + # Not a hypothetical. cmux exports `CMUX_SOCKET=` empty alongside a + # populated `CMUX_SOCKET_PATH` in the same environment, so a resolution + # that read the first set *variable* rather than the first set *value* + # would find no socket at all — and would then conclude the session is not + # running inside cmux, because the socket is what says so. FALSEY = {"", "0", "false", "no", "off"} DEFAULT_TIMEOUT = 1.0 @@ -31,12 +54,14 @@ module Smith # become nil, so `socket_path = ""` in a config file is the same as the key # not being there — otherwise the empty value would shadow the environment # with nothing. + # + # `enabled` keeps its third state for the same reason: absent is "nobody + # said", and that is the answer the environment gets to overrule. def self.from_table(table : Hash(String, TOML::Any)? = nil) : NotifyConfig - enabled = setting(table, "enabled").try(&.as_bool?) timeout = float_setting(table, "timeout") NotifyConfig.new( - enabled: enabled.nil? ? false : enabled, + enabled: setting(table, "enabled").try(&.as_bool?), socket_path: normalize(setting(table, "socket_path").try(&.as_s?)), surface_id: normalize(setting(table, "surface_id").try(&.as_s?)), workspace_id: normalize(setting(table, "workspace_id").try(&.as_s?)), @@ -51,27 +76,25 @@ module Smith # # `env` is a parameter rather than `ENV` so the resolution is testable # without touching the process environment. - def self.resolve(config : NotifyConfig, env : Hash(String, String?) = ENV) : NotifyConfig - cmux = truthy(env, "CMUX") + def self.resolve(config : NotifyConfig, env : Hash(String, String?) = env_snapshot) : NotifyConfig + # A socket cmux itself put into the environment, as opposed to one a + # config file named. Kept apart because the two mean different things: + # the first says "this terminal is inside cmux, right now", the second + # only says "here is a location". + live_socket = socket_from_env(env) NotifyConfig.new( - # `CMUX` being *truthy* is cmux announcing "you are inside me", which - # is the same statement as `enabled = true` — and the one made about - # the terminal actually in use. A falsey `CMUX` is one rule for every - # variable: an absent value. It says nothing about notifications, so - # the config file keeps deciding; turning them off from inside cmux is - # `enabled = false`, which this honours. - enabled: cmux.nil? ? config.enabled : true, - socket_path: socket_path(env, config.socket_path), - surface_id: presence(env, "CMUX_SURFACE_ID") || config.surface_id, - workspace_id: presence(env, "CMUX_WORKSPACE_ID") || config.workspace_id, + enabled: enabled(config, live_socket), + socket_path: live_socket || config.socket_path, + surface_id: first_of(env, SURFACE_ID_KEYS) || config.surface_id, + workspace_id: first_of(env, WORKSPACE_ID_KEYS) || config.workspace_id, timeout: config.timeout ) end # Resolve, then build. The one call a caller that is not itself resolving # anything needs. - def self.build(config : NotifyConfig, env : Hash(String, String?) = ENV) : CmuxClientable + def self.build(config : NotifyConfig, env : Hash(String, String?) = env_snapshot) : CmuxClientable client(resolve(config, env)) end @@ -91,17 +114,53 @@ module Smith NullCmuxClient.new end - private def self.socket_path(env : Hash(String, String?), configured : String?) : String? + # Whether notifications go out. Three answers, because there are three + # questions and only two of them belong to the config file: + # + # An explicit `enabled = false` is honoured no matter what the terminal + # says — that is somebody turning them off, and being inside cmux is not a + # reason to overrule them. An explicit `true` is honoured as readily. + # + # Absent is nobody having said, and that is where the terminal gets its + # say: cmux announcing a socket *is* the announcement that this session is + # running inside it, which is the situation a completion notification + # exists for. So the default is on inside cmux and off everywhere else, + # and a run started in a plain terminal is unchanged by this feature. + # + # Deliberately not decided by whether a socket path resolved *from config*: + # pointing at a location is not the same statement as "you are inside me", + # and treating it as one would switch notifications on for a config file + # that only ever meant to say where the socket is. + private def self.enabled(config : NotifyConfig, live_socket : String?) : Bool + explicit = config.enabled + return explicit unless explicit.nil? + + !live_socket.nil? + end + + private def self.socket_from_env(env : Hash(String, String?)) : String? SOCKET_PATH_KEYS.each do |key| value = truthy(env, key) next if value.nil? - # `CMUX` is a flag first and a path second: only when it holds - # something that looks like a location is it read as one. + # A bare `CMUX` holds a flag in the wild — `CMUX=1` — and reading that + # as a location would connect to a file called `1` in the current + # directory. Only something shaped like a path is taken as one. next if key == "CMUX" && !value.includes?("/") return value end - configured + nil + end + + # The first of `keys` that holds a value saying something, so a documented + # spelling can stand in for an exported one without either being assumed. + private def self.first_of(env : Hash(String, String?), keys : Enumerable(String)) : String? + keys.each do |key| + value = presence(env, key) + return value unless value.nil? + end + + nil end private def self.setting(table : Hash(String, TOML::Any)?, key : String) : TOML::Any? @@ -132,5 +191,18 @@ module Smith stripped = value.strip stripped.empty? ? nil : stripped end + + # A copy of the process environment, in the type the resolution works in. + # + # Not `ENV` itself as the default: `ENV` is not a `Hash`, and a default + # argument is only checked where the method is actually called — so + # `= ENV` sat unobjected until something called `resolve`, and it broke the + # build rather than failing quietly. Snapshotting also means a resolution + # cannot observe the environment changing underneath it mid-call. + private def self.env_snapshot : Hash(String, String?) + snapshot = Hash(String, String?).new + ENV.each { |key, value| snapshot[key] = value } + snapshot + end end end diff --git a/src/smith/notify_config.cr b/src/smith/notify_config.cr index e7b6abf..7e66e5d 100644 --- a/src/smith/notify_config.cr +++ b/src/smith/notify_config.cr @@ -5,8 +5,15 @@ module Smith # # Blank strings are normalised to `nil` so callers can treat "unset" and # "explicitly empty" the same way. + # + # `enabled` is a `Bool?` for the same reason, and it matters more here than + # anywhere else in this record: `false` is somebody turning notifications + # off, `nil` is nobody having said. Only the second one lets the terminal + # have a say — see `CmuxClient.resolve`. Collapsing the two would mean a + # config file that never mentions `[notify]` was read as one that refused + # it, and no amount of environment could switch them back on. record NotifyConfig, - enabled : Bool = false, + enabled : Bool? = nil, socket_path : String? = nil, surface_id : String? = nil, workspace_id : String? = nil, @@ -17,9 +24,15 @@ module Smith !@socket_path.nil? end + # True when notifications were asked for. `nil` is not: nobody asked, and + # this record on its own has nothing to go on. + def enabled? : Bool + @enabled == true + end + # True when everything needed to actually deliver is present. def deliverable? : Bool - @enabled && socket? + enabled? && socket? end end end From 7f191708b864be78b7d5c092af6de2a5c143a610 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mathias=20Karst=C3=A4dt?= Date: Wed, 9 Sep 2026 01:35:34 +0200 Subject: [PATCH 3/5] feat: notify on turn completion, and still deliver nothing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit TurnCompleted now has a consumer: a listener that collects what the run said and, when the turn is over, hands it to Smith::Notify as the three fields cmux documents — title, subtitle, body. Subtitle names the project the session belongs to and its name where it has one, which is what tells two smith runs apart from another tab. A listener of its own rather than a branch in the renderer: which renderer is showing the run is irrelevant to whether a notification should go out — a --json caller and a fullscreen one are equally served — and a renderer owns the run's exit code and failure state, which this has no business deciding. It is attached in build_agent, the one place all five routes to a main-thread agent pass through, so the plain loop, the fullscreen one, `run`, `resume` and a mid-session `/resume` all notify without any of them knowing. Subagents are left out, and not by a check for one: children are built by Subagents::Supervisor with their own Agent and their own listener, so they never meet this one. That is also the right answer — cmux agrees, to the extent that it suppresses subagent completions of its own agents — though its CMUX_SUPPRESS_SUBAGENT_NOTIFICATIONS governs events cmux derives from the wrappers it installs for Claude Code and Codex, not anything posted over the socket, so smith cannot lean on it here. Two things the specs found while writing them: - Text collected for a turn that ended in failure — TurnError, BudgetExceeded, ContextExhausted, none of which is followed by TurnCompleted — was left standing and prefixed to the next turn's answer. A session of two turns notified the second body with the first run's half-answer glued in front. Reproduced and asserted for all three. - The body was not cleared after sending, so a session of four turns sent the fourth one holding all four. The body is collapsed to one line and cut at a word boundary with an ellipsis, counting characters rather than bytes so German text cannot be split mid-character. MAX_BODY is smith's own choice: cmux documents no length on any of the three fields. It fires on every turn and does not guess whether anybody is watching, because cmux already withdraws the banner of a workspace that has become visible — the focus decision belongs to the terminal, which is the one thing this cannot see from inside. Still delivers nothing, which is the point of stopping here: CmuxClient.client returns NullCmuxClient until the socket is spoken to (#120). The wiring is complete and every notification is dropped at that seam, asserted rather than assumed, so build_agent can attach the listener unconditionally instead of branching on "am I inside cmux?". Co-Authored-By: Claude Opus 5 --- spec/smith/cmux_notify_spec.cr | 334 +++++++++++++++++++++++++++++++++ src/smith/cli.cr | 31 +++ src/smith/turn_notifier.cr | 104 ++++++++++ 3 files changed, 469 insertions(+) create mode 100644 src/smith/turn_notifier.cr diff --git a/spec/smith/cmux_notify_spec.cr b/spec/smith/cmux_notify_spec.cr index fe3eaab..4e902fa 100644 --- a/spec/smith/cmux_notify_spec.cr +++ b/spec/smith/cmux_notify_spec.cr @@ -2,6 +2,11 @@ require "socket" require "../spec_helper" require "../../src/smith/cmux_client" require "../../src/smith/notify" +require "../../src/smith/turn_notifier" +require "../../src/smith/agent" +require "../../src/smith/tools" +require "../../src/smith/cli" +require "../../src/smith/session" # Records what `Smith::Notify` hands over, so the payload rules can be asserted # without a cmux daemon — which is the point: nothing here should depend on @@ -500,3 +505,332 @@ describe Smith::Notify do client.last.to_json.should eq(%({"type":"notification","title":"Build done","subtitle":"2 failed"})) end end + +# What the first consumer does with a run. Driven through the real agent loop +# wherever a loop exists to drive, because the point of this listener is that +# it reacts to events as they arrive — a hand-written sequence would pass +# whatever order it happened to assert. +private class NotifyingProvider < Smith::LLM::Provider + getter calls = 0 + + def name : String + "mock" + end + + def default_model : String + "mock-model" + end + + def complete(request : Smith::LLM::Request) : Smith::LLM::Response + @calls += 1 + + if @calls == 1 + # Turn one announces and calls a tool — the announcement is not the + # answer, and the run is not over. + blocks = [ + Smith::LLM::ContentBlock.text("Let me look at that."), + Smith::LLM::ContentBlock.tool_use("call_1", "read_file", JSON.parse(%({"path": "spec/spec_helper.cr"}))), + ] + else + blocks = [ + Smith::LLM::ContentBlock.text("Done. The tests pass."), + ] + end + + Smith::LLM::Response.new("resp_#{@calls}", request.model, blocks, usage: Smith::LLM::Usage.new(10, 5, 15)) + end +end + +describe Smith::TurnNotifier do + it "says nothing until the turn is over" do + client = RecordingClient.new + notifier = Smith::TurnNotifier.new(Smith::Notify.new(client)) + + notifier.handle(Smith::Events::AssistantText.new("thinking out loud")) + client.payloads.should be_empty + + notifier.handle(Smith::Events::TurnCompleted.new(1)) + client.payloads.size.should eq(1) + end + + it "names the run and carries where it came from" do + client = RecordingClient.new + notifier = Smith::TurnNotifier.new(Smith::Notify.new(client), subtitle: "smith · notify") + + notifier.handle(Smith::Events::AssistantText.new("all green")) + notifier.handle(Smith::Events::TurnCompleted.new(3)) + + payload = client.last + payload["title"].should eq("Smith") + payload["subtitle"].should eq("smith · notify") + payload["body"].should eq("all green") + end + + it "reports the answer, not the announcement that came before the tools" do + # What a model says before calling a tool is a promise of work, not a + # result. Sent as the body it would read, an hour later, as though the run + # had stopped mid-sentence. + client = RecordingClient.new + notifier = Smith::TurnNotifier.new(Smith::Notify.new(client)) + + notifier.handle(Smith::Events::AssistantText.new("Let me look at that.")) + notifier.handle(Smith::Events::ToolStart.new("call_1", "read_file", JSON.parse("{}"))) + notifier.handle(Smith::Events::ToolFinished.new("call_1", "read_file", "contents", false)) + notifier.handle(Smith::Events::AssistantText.new("Done.")) + notifier.handle(Smith::Events::TurnCompleted.new(2)) + + client.last["body"].should eq("Done.") + end + + it "sends no body at all for a run that ended among its tools" do + client = RecordingClient.new + notifier = Smith::TurnNotifier.new(Smith::Notify.new(client)) + + notifier.handle(Smith::Events::AssistantText.new("Let me look at that.")) + notifier.handle(Smith::Events::ToolStart.new("call_1", "read_file", JSON.parse("{}"))) + notifier.handle(Smith::Events::TurnCompleted.new(1)) + + client.last.has_key?("body").should be_false + end + + it "collapses a multi-paragraph answer into one line" do + client = RecordingClient.new + notifier = Smith::TurnNotifier.new(Smith::Notify.new(client)) + + notifier.handle(Smith::Events::AssistantText.new("First paragraph.\n\nSecond one,\twith a tab.")) + notifier.handle(Smith::Events::TurnCompleted.new(1)) + + client.last["body"].should eq("First paragraph. Second one, with a tab.") + end + + it "cuts a long answer at a word boundary and says it did" do + client = RecordingClient.new + notifier = Smith::TurnNotifier.new(Smith::Notify.new(client)) + + long = Array.new(40) { |i| "word#{i}" }.join(" ") + notifier.handle(Smith::Events::AssistantText.new(long)) + notifier.handle(Smith::Events::TurnCompleted.new(1)) + + body = client.last["body"].as_s + body.size.should be <= Smith::TurnNotifier::MAX_BODY + 1 + body.ends_with?("…").should be_true + + # The cut happened at a space rather than inside a word: what is left is + # one whole token, and the source had none of any other shape. + kept = body[0, body.size - 1] + kept.split(" ").last.should match(/^word\d+$/) + # …and it dropped something rather than merely trailing off. + kept.split(" ").size.should be < long.split(" ").size + end + + it "counts characters rather than bytes, so a body cannot be cut in half" do + # German text is where this shows: a byte-oriented cut would split a + # two-byte character and send an invalid string. + client = RecordingClient.new + notifier = Smith::TurnNotifier.new(Smith::Notify.new(client)) + + long = "Grüße " * 60 + notifier.handle(Smith::Events::AssistantText.new(long)) + notifier.handle(Smith::Events::TurnCompleted.new(1)) + + body = client.last["body"].as_s + body.size.should be <= Smith::TurnNotifier::MAX_BODY + 1 + body.valid_encoding?.should be_true + end + + it "leaves an answer that fits alone, ellipsis and all" do + client = RecordingClient.new + notifier = Smith::TurnNotifier.new(Smith::Notify.new(client)) + + notifier.handle(Smith::Events::AssistantText.new("short")) + notifier.handle(Smith::Events::TurnCompleted.new(1)) + + client.last["body"].should eq("short") + end + + it "notifies once per turn, and again for the next one" do + client = RecordingClient.new + notifier = Smith::TurnNotifier.new(Smith::Notify.new(client)) + + 2.times do |i| + notifier.handle(Smith::Events::AssistantText.new("turn #{i}")) + notifier.handle(Smith::Events::TurnCompleted.new(i + 1)) + end + + client.payloads.size.should eq(2) + client.payloads[0]["body"].should eq("turn 0") + client.payloads[1]["body"].should eq("turn 1") + end + + it "drops what a run said before it failed, so the next run starts clean" do + # A run does not have to end on a completed turn: a provider that fails, a + # budget that runs out and a window that fills each end one, and none of + # them is followed by `TurnCompleted`. Whatever was collected belongs to + # the run that died, and left standing it would be prefixed to the answer + # of the next one — a session of two turns would notify "second answer" + # with the first turn's half-answer glued in front of it. + [ + Smith::Events::TurnError.new("Provider completion failed"), + Smith::Events::BudgetExceeded.new(spent_usd: 2.0, limit_usd: 1.0), + Smith::Events::ContextExhausted.new(9000, 8000, 0), + ].each do |ending| + client = RecordingClient.new + notifier = Smith::TurnNotifier.new(Smith::Notify.new(client)) + + notifier.handle(Smith::Events::AssistantText.new("the run that died")) + notifier.handle(ending) + client.payloads.should be_empty, "#{ending.class} notified" + + notifier.handle(Smith::Events::AssistantText.new("the answer after it")) + notifier.handle(Smith::Events::TurnCompleted.new(1)) + + client.last["body"].should eq("the answer after it"), "#{ending.class} leaked" + end + end + + it "ignores everything that is not a turn ending" do + client = RecordingClient.new + notifier = Smith::TurnNotifier.new(Smith::Notify.new(client)) + + notifier.handle(Smith::Events::ToolStart.new("call_1", "bash", JSON.parse("{}"))) + notifier.handle(Smith::Events::UsageUpdated.new(Smith::LLM::Usage.new(1, 1, 2))) + notifier.handle(Smith::Events::TurnError.new("provider said no")) + notifier.handle(Smith::Events::BudgetExceeded.new(spent_usd: 1.5, limit_usd: 1.0)) + + client.payloads.should be_empty + end + + it "says nothing through a null client, and survives a client that throws" do + # Not inside cmux is the ordinary case: a plain terminal run must not be + # changed by this feature, and must not pay for it either. + null_notifier = Smith::TurnNotifier.new(Smith::Notify.new(Smith::NullCmuxClient.new)) + null_notifier.handle(Smith::Events::TurnCompleted.new(1)) + + # A notification is the last thing a run does; it must not be the reason + # one ends. This call is the assertion: a client that throws out of + # `handle` fails the example right here. + exploding = Smith::TurnNotifier.new(Smith::Notify.new(ExplodingClient.new)) + exploding.handle(Smith::Events::TurnCompleted.new(1)) + end + + describe "through the real agent loop" do + it "is told by the events a run actually emits" do + provider = NotifyingProvider.new + registry = Smith::Tools::Registry.default + agent = Smith::Agent.new(provider: provider, registry: registry, model: "mock-model") + + client = RecordingClient.new + notifier = Smith::TurnNotifier.new(Smith::Notify.new(client), subtitle: "spec-project") + + # The same two lines `CLI#build_agent` runs, which is the wiring under + # test: a listener alongside the renderer rather than inside it. + agent.on_event { |event| notifier.handle(event) } + agent.send("Read spec_helper and tell me when the tests pass") + + # One notification for the whole run — the intermediate turn that called + # a tool announced itself and was not over. + client.payloads.size.should eq(1) + payload = client.last + payload["type"].should eq("notification") + payload["title"].should eq("Smith") + payload["subtitle"].should eq("spec-project") + payload["body"].should eq("Done. The tests pass.") + end + end +end + +# The wiring itself: what `CLI#build_agent` attaches, and what a resolved +# notification says about where the run is. Reaching into the private helpers +# rather than restating them is the same reason clear_persist_spec.cr does — +# a copy written out in the spec would pass whatever the CLI happened to do. +class Smith::CLI + def notify_for_spec : Smith::Notify + notify + end + + def notify_subtitle_for_spec(session : Smith::Session::Data?) : String? + notify_subtitle(session) + end + + def config_for_spec : Smith::Config + @config + end +end + +describe "the notifications a CLI run is wired to send" do + it "resolves to nothing to deliver to in a shell that is not cmux" do + # The default has to be "a plain terminal run is unchanged": nothing is + # delivered, and nothing is even looked for. Resolved against an + # environment passed in rather than the ambient one — these specs run + # inside a real cmux terminal more often than not, and a test that reads + # `ENV` would then assert the opposite of what it claims. + temp_dir = File.join(Dir.tempdir, "smith_wire_#{Random::Secure.hex(4)}") + previous = ENV["SMITH_HOME"]? + ENV["SMITH_HOME"] = temp_dir + + begin + cli = Smith::CLI.new([] of String) + + resolved = Smith::CmuxClient.resolve(cli.config_for_spec.notify, {} of String => String?) + resolved.enabled?.should be_false + resolved.deliverable?.should be_false + ensure + previous ? (ENV["SMITH_HOME"] = previous) : ENV.delete("SMITH_HOME") + FileUtils.rm_rf(temp_dir) + end + end + + it "delivers nothing yet, inside cmux or not" do + # The seam this stops at, asserted rather than assumed: `Notify#enabled?` + # reports whether there is a client that can deliver, and until the socket + # is spoken to there is only the null one. So the wiring is complete and + # still sends nothing — which is why `build_agent` can attach it + # unconditionally instead of branching on "am I inside cmux?". + temp_dir = File.join(Dir.tempdir, "smith_wire_#{Random::Secure.hex(4)}") + previous = ENV["SMITH_HOME"]? + ENV["SMITH_HOME"] = temp_dir + + begin + cli = Smith::CLI.new([] of String) + notify = cli.notify_for_spec + + notify.enabled?.should be_false + # …and a run finishing still costs nothing and still fails nothing. + notifier = Smith::TurnNotifier.new(notify) + notifier.handle(Smith::Events::TurnCompleted.new(1)) + ensure + previous ? (ENV["SMITH_HOME"] = previous) : ENV.delete("SMITH_HOME") + FileUtils.rm_rf(temp_dir) + end + end + + it "names the project a session was started in" do + # Read off the session rather than `Dir.current`: a resumed session runs + # wherever it was created, and that is the name worth reading from another + # tab. Built rather than created, because `Store#create` writes a session + # file — and a spec has no business leaving one in the developer's + # `~/.smith`. + session = Smith::Session::Data.new(id: "spec-session", cwd: "/work/smith", model: "mock-model", provider: "mock") + + Smith::CLI.new([] of String).notify_subtitle_for_spec(session).should eq("smith") + end + + it "prefers a session name when it has one" do + session = Smith::Session::Data.new( + id: "spec-session", + cwd: "/work/smith", + model: "mock-model", + provider: "mock", + name: "notify-work" + ) + + Smith::CLI.new([] of String).notify_subtitle_for_spec(session).should eq("smith · notify-work") + end + + it "still names something when there is no session yet" do + # A headless run has one, but the helper must not depend on it: it is + # called from the same place the agent is built, and a nil session is a + # state that reaches it. + Smith::CLI.new([] of String).notify_subtitle_for_spec(nil).should eq(File.basename(Dir.current)) + end +end diff --git a/src/smith/cli.cr b/src/smith/cli.cr index 23a1115..ca4c2dd 100644 --- a/src/smith/cli.cr +++ b/src/smith/cli.cr @@ -27,6 +27,7 @@ require "./update" require "./doctor" require "./marketplace" require "./ui" +require "./turn_notifier" module Smith class CLI @@ -73,6 +74,7 @@ module Smith # keep the plain renderer even on a TTY. @interactive_tui : Bool = false @tui_app : UI::App? = nil + @notify : Notify? = nil @tui_warned : Bool = false @update_check : Bool = false @allow_unverified : Bool = false @@ -684,6 +686,25 @@ module Smith end end + # Where completion notifications go. Built once, from the config and the + # `CMUX_*` environment merged in `CmuxClient.resolve`, and a no-op anywhere + # cmux is not the terminal in use — so no call site has to ask first. + private def notify : Notify + @notify ||= Notify.new(CmuxClient.build(@config.notify)) + end + + # Which session this is, in one glance from another tab. The project + # directory is the part that tells two smith runs apart, since they are + # usually the same project run twice rather than two projects; a session + # that was named says its name, because that is what the user would have + # called it. + private def notify_subtitle(session_data : Session::Data?) : String? + project = File.basename(session_data.try(&.cwd) || Dir.current) + name = session_data.try(&.name) + + name.nil? || name.empty? ? project : "#{project} · #{name}" + end + # Takes the whole session rather than its pieces: passing messages and the # calibration ratio separately is how one call site came to carry the # transcript without what had been learned about measuring it. @@ -817,6 +838,16 @@ module Smith renderer.handle(event) end + # A second listener rather than a branch inside the first: a + # notification is not a rendering choice, and every renderer — plain, + # JSON, fullscreen — is served by the same one. Fresh per agent, so the + # subtitle is the session this agent runs and the text collected for it + # cannot carry over into one that replaced it. + notifier = TurnNotifier.new(notify, subtitle: notify_subtitle(session_data)) + agent.on_event do |event| + notifier.handle(event) + end + plan = plan_session plan.on_plan = ->(text : String) do renderer.handle(Events::PlanPresented.new(text)) diff --git a/src/smith/turn_notifier.cr b/src/smith/turn_notifier.cr new file mode 100644 index 0000000..8097c21 --- /dev/null +++ b/src/smith/turn_notifier.cr @@ -0,0 +1,104 @@ +require "./events" +require "./notify" + +module Smith + # The first consumer of `Smith::Notify`: it listens to a run and turns "this + # turn is over" into one notification. + # + # A listener of its own rather than a fourth renderer, because which + # renderer is showing the run is irrelevant to whether one should go out — a + # `--json` caller and a fullscreen one are equally served by being told when + # to look back — and because a renderer owns the exit code and the failure + # state of the run, which this has no business deciding. + # + # Attached in `CLI#build_agent`, the one place every route to a main-thread + # agent passes through, so the plain loop, the fullscreen one, `run`, + # `resume` and a mid-session `/resume` all notify without any of them having + # to know. + # + # A subagent is deliberately left out, and not by a check for it: children + # are built by `Subagents::Supervisor` with `Agent.new` of its own and given + # their own listener, so they never meet this one. That is also the right + # answer — a delegate that announced each child as it finished would be + # noise where the parent's own completion is the signal. cmux agrees, to the + # extent that it suppresses subagent completions of its own agents; note that + # its `CMUX_SUPPRESS_SUBAGENT_NOTIFICATIONS` governs events cmux derives from + # the wrappers it installs for Claude Code and Codex, not anything posted + # over the socket, so smith cannot lean on it here. + # + # It fires on every turn rather than only on long ones, and deliberately does + # not try to guess whether anybody is watching: cmux already withdraws the + # banner of a workspace that has become visible, so the decision belongs to + # the terminal that knows its own focus — which is the one thing this cannot + # see from inside. + class TurnNotifier + # A body is read at a glance, from another tab, while deciding whether to + # switch back. What does not fit that is not a body: a model's closing + # answer runs to pages, and a notification carrying three of them is one + # nobody reads. + # + # Smith's own limit — cmux documents a title, a subtitle and a body + # without a length on any of them, so nothing here is imposed by the + # terminal. Cut at a word boundary and marked as cut, rather than sent + # whole or dropped: the reader should get the sentence the run ended on + # and be told there is more. + MAX_BODY = 200 + + def initialize(@notify : Notify, @subtitle : String? = nil) + @text = "" + end + + def handle(event : Events::Event) : Nil + case event + when Events::AssistantText + # Collected rather than sent: one response can carry several text + # blocks, and only what the last one said is the answer. + @text += event.text + when Events::ToolStart + # Text before a tool call was an announcement — "let me look at that + # file" — not an answer. Dropped, so a run that ends among its tools + # reports no body rather than a stale promise of one. + @text = "" + when Events::TurnCompleted + announce + when Events::TurnError, Events::BudgetExceeded, Events::ContextExhausted + # A run can end on any of these instead of a completed turn — a + # provider that failed, a budget that ran out, a window that filled. + # Whatever text was collected belongs to the run that just died, and + # left standing it would be prefixed to the next turn's answer. + @text = "" + end + end + + # The turn is over. Title names who, subtitle names where, and the body + # says what came out — the three fields `cmux notify` documents, and + # nothing beyond them: what else a payload may carry is a question for the + # wire, which does not exist yet (#120). + private def announce : Nil + begin + @notify.notify("Smith", subtitle: @subtitle, body: body) + ensure + # The text belonged to the turn that just ended. Left standing it would + # be prefixed to the next one's answer, and a session of four turns + # would notify four times with the fourth body holding all four. + @text = "" + end + end + + # One line of prose, cut at a word boundary. Whitespace is collapsed + # because a model's answer usually starts with a paragraph, and a body + # carrying twelve newlines reads as a gap rather than as a message. + private def body : String? + collapsed = @text.gsub(/\s+/, " ").strip + return nil if collapsed.empty? + return collapsed if collapsed.size <= MAX_BODY + + cut = collapsed[0, MAX_BODY] + if (space = cut.rindex(' ')) && space > MAX_BODY // 2 + cut = cut[0, space] + end + + "#{cut}…" + end + end +end From 7a4750d4674db254677cb9ce9c855951e63cf070 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mathias=20Karst=C3=A4dt?= Date: Wed, 9 Sep 2026 01:51:46 +0200 Subject: [PATCH 4/5] refactor: one door into the notify subsystem, not two MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The CLI built its notifier by naming CmuxClient as well as Notify — calling the resolver, then wrapping the client it got back. That made two doors where the caller only needs one, and the second door is the one that knows about the environment, the socket and the wire. Notify.build takes the resolved config and returns a notifier, so a caller hands over what the config file said and gets back something that notifies or silently does not. Which client that deserves is not the caller's question. The resolution stays where it is — Notify still knows no environment, no socket and no protocol, and only names the module that does. Co-Authored-By: Claude Opus 5 --- src/smith/cli.cr | 9 +++++---- src/smith/notify.cr | 17 +++++++++++++++++ 2 files changed, 22 insertions(+), 4 deletions(-) diff --git a/src/smith/cli.cr b/src/smith/cli.cr index ca4c2dd..9bda220 100644 --- a/src/smith/cli.cr +++ b/src/smith/cli.cr @@ -686,11 +686,12 @@ module Smith end end - # Where completion notifications go. Built once, from the config and the - # `CMUX_*` environment merged in `CmuxClient.resolve`, and a no-op anywhere - # cmux is not the terminal in use — so no call site has to ask first. + # Where completion notifications go. Built once, and a no-op anywhere cmux + # is not the terminal in use — so no call site has to ask first. Which + # config and which environment decide that is `Notify`'s business, and the + # reason this names one class rather than two. private def notify : Notify - @notify ||= Notify.new(CmuxClient.build(@config.notify)) + @notify ||= Notify.build(@config.notify) end # Which session this is, in one glance from another tab. The project diff --git a/src/smith/notify.cr b/src/smith/notify.cr index 2e0b4f1..9b5f768 100644 --- a/src/smith/notify.cr +++ b/src/smith/notify.cr @@ -1,6 +1,8 @@ require "json" +require "./cmux_client" require "./cmux_clientable" require "./null_cmux_client" +require "./notify_config" module Smith # Decides that a notification should go out, and what it says. Nothing @@ -28,6 +30,21 @@ module Smith def initialize(@client : CmuxClientable) end + # The one door the rest of smith goes through: hand over the resolved + # config, get back something that notifies or silently does not. No caller + # has to ask which client a config deserves, whether a socket is involved + # or whether cmux is the terminal in use — and so no caller learns the name + # of anything that knows, which is what keeps "the rest of smith knows + # none of these three" true rather than approximately true. + # + # The resolution lives in `CmuxClient`, not here: this class still knows no + # environment, no socket and no protocol, and only names the thing that + # does. A constructor taking a client stays, because that is how a spec + # hands over a recording one. + def self.build(config : NotifyConfig) : Notify + new(CmuxClient.build(config)) + end + # True when there is somewhere to deliver to. Purely informational — the # caller does not need it to call `notify`, and should not skip on it: a # no-op is exactly what "not inside cmux" means here. From b3544febf09e9efcaccca7042d1431451db2545f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mathias=20Karst=C3=A4dt?= Date: Wed, 9 Sep 2026 01:59:57 +0200 Subject: [PATCH 5/5] refactor: read the [notify] table where the record lives, not where the environment does MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #120 asks that the rest of smith name none of the client names but `Notify`, and `Config#notify` still named `CmuxClient` — the module that owns the environment, the socket and the wire — in order to turn a TOML table into a record. Reading a config file is none of those three things, and the argument that this was "about concepts rather than names" was a way of leaving a checklist item undone. `from_table` moves onto `NotifyConfig`, which is the record it builds. The shared blank-string rule moves with it and the environment side delegates to it, so "unset" and "explicitly empty" stay one rule across both tiers rather than becoming two that could drift. `CmuxClient` drops its `toml` require and keeps what it is for: the `CMUX_*` variables, which of the two wins, and the client a resolved config deserves. Now verified rather than intended: outside the subsystem's own files, smith names `CmuxClient`, `CmuxClientable` and `NullCmuxClient` nowhere at all, and `ENV` appears in exactly one file. Co-Authored-By: Claude Opus 5 --- spec/smith/cmux_notify_spec.cr | 20 ++++++------ src/smith/cmux_client.cr | 53 +++++--------------------------- src/smith/config.cr | 16 +++++----- src/smith/notify_config.cr | 56 +++++++++++++++++++++++++++++++--- 4 files changed, 77 insertions(+), 68 deletions(-) diff --git a/spec/smith/cmux_notify_spec.cr b/spec/smith/cmux_notify_spec.cr index 4e902fa..14fcfc5 100644 --- a/spec/smith/cmux_notify_spec.cr +++ b/spec/smith/cmux_notify_spec.cr @@ -88,9 +88,7 @@ describe Smith::NotifyConfig do it "defaults the timeout to something a socket can live with" do Smith::NotifyConfig.new.timeout.should eq(1.0) end -end -describe Smith::CmuxClient do describe ".from_table" do it "reads the [notify] keys" do table = TOML.parse(<<-TOML) @@ -101,7 +99,7 @@ describe Smith::CmuxClient do timeout = 2.5 TOML - config = Smith::CmuxClient.from_table(table) + config = Smith::NotifyConfig.from_table(table) config.enabled.should be_true config.socket_path.should eq("/tmp/cmux.sock") @@ -111,7 +109,7 @@ describe Smith::CmuxClient do end it "says nothing when there is no section at all" do - config = Smith::CmuxClient.from_table(nil) + config = Smith::NotifyConfig.from_table(nil) config.enabled.should be_nil config.socket_path.should be_nil @@ -121,12 +119,12 @@ describe Smith::CmuxClient do it "reads an integer timeout as well as a float one" do table = TOML.parse("timeout = 3") - Smith::CmuxClient.from_table(table).timeout.should eq(3.0) + Smith::NotifyConfig.from_table(table).timeout.should eq(3.0) end it "treats a timeout that could not work as the default" do - Smith::CmuxClient.from_table(TOML.parse("timeout = 0")).timeout.should eq(1.0) - Smith::CmuxClient.from_table(TOML.parse("timeout = -1.5")).timeout.should eq(1.0) + Smith::NotifyConfig.from_table(TOML.parse("timeout = 0")).timeout.should eq(1.0) + Smith::NotifyConfig.from_table(TOML.parse("timeout = -1.5")).timeout.should eq(1.0) end it "turns blank strings into unset, so they cannot shadow the environment" do @@ -136,7 +134,7 @@ describe Smith::CmuxClient do surface_id = "" TOML - config = Smith::CmuxClient.from_table(table) + config = Smith::NotifyConfig.from_table(table) config.socket_path.should be_nil config.surface_id.should be_nil @@ -151,7 +149,7 @@ describe Smith::CmuxClient do socket_path = 42 TOML - config = Smith::CmuxClient.from_table(table) + config = Smith::NotifyConfig.from_table(table) # Not false: an unreadable `enabled` is nobody having said, so the # terminal keeps its say rather than a typo switching notifications off. @@ -165,13 +163,15 @@ describe Smith::CmuxClient do surface_id = " surface:1 " TOML - config = Smith::CmuxClient.from_table(table) + config = Smith::NotifyConfig.from_table(table) config.socket_path.should eq("/tmp/cmux.sock") config.surface_id.should eq("surface:1") end end +end +describe Smith::CmuxClient do describe ".resolve" do it "leaves the config alone when the environment says nothing" do resolved = Smith::CmuxClient.resolve(filled_config, no_cmux_env) diff --git a/src/smith/cmux_client.cr b/src/smith/cmux_client.cr index 976594a..c37788a 100644 --- a/src/smith/cmux_client.cr +++ b/src/smith/cmux_client.cr @@ -1,15 +1,15 @@ -require "toml" require "./cmux_clientable" require "./null_cmux_client" require "./notify_config" module Smith - # Turns the two places a cmux notification setup can be described — the - # `[notify]` section of config.toml and the `CMUX_*` environment the cmux - # terminal exports into every process it spawns — into one `NotifyConfig`, - # and that into something `Smith::Notify` can talk to. + # The environment half of a cmux notification setup. `NotifyConfig.from_table` + # reads what the config file said; this merges in the `CMUX_*` variables the + # terminal exports into every process it spawns, decides which of the two + # wins, and turns the result into something `Smith::Notify` can talk to. # - # This is the only place that knows those names. `Smith::Notify` sees a + # This is the only place in smith that knows those variable names, that a + # socket is involved, or that a protocol exists. `Smith::Notify` sees a # resolved config and a `CmuxClientable`; neither knows there is an # environment, and neither reaches for one. module CmuxClient @@ -48,27 +48,6 @@ module Smith # running inside cmux, because the socket is what says so. FALSEY = {"", "0", "false", "no", "off"} - DEFAULT_TIMEOUT = 1.0 - - # The config tier, before the environment has had a say. Blank strings - # become nil, so `socket_path = ""` in a config file is the same as the key - # not being there — otherwise the empty value would shadow the environment - # with nothing. - # - # `enabled` keeps its third state for the same reason: absent is "nobody - # said", and that is the answer the environment gets to overrule. - def self.from_table(table : Hash(String, TOML::Any)? = nil) : NotifyConfig - timeout = float_setting(table, "timeout") - - NotifyConfig.new( - enabled: setting(table, "enabled").try(&.as_bool?), - socket_path: normalize(setting(table, "socket_path").try(&.as_s?)), - surface_id: normalize(setting(table, "surface_id").try(&.as_s?)), - workspace_id: normalize(setting(table, "workspace_id").try(&.as_s?)), - timeout: timeout.nil? || timeout <= 0 ? DEFAULT_TIMEOUT : timeout - ) - end - # Config plus environment, environment winning. Inside cmux the variables # describe the terminal that is running right now — this surface, this # workspace, this socket — so they are the more accurate answer than @@ -163,33 +142,17 @@ module Smith nil end - private def self.setting(table : Hash(String, TOML::Any)?, key : String) : TOML::Any? - table.try(&.[key]?) - end - - # TOML writes `timeout = 2` as an integer and `timeout = 0.5` as a float, - # and both are the same statement — `as_f?` reads either. - private def self.float_setting(table : Hash(String, TOML::Any)?, key : String) : Float64? - setting(table, key).try(&.as_f?) - end - # A set variable that says something. Whitespace-only and the usual # spellings of "off" come back as nil, which is what lets the tier below # speak. private def self.truthy(env : Hash(String, String?), key : String) : String? - value = normalize(env[key]?) + value = NotifyConfig.normalize(env[key]?) return nil if value.nil? FALSEY.includes?(value.downcase) ? nil : value end private def self.presence(env : Hash(String, String?), key : String) : String? - normalize(env[key]?) - end - - private def self.normalize(value : String?) : String? - return nil if value.nil? - stripped = value.strip - stripped.empty? ? nil : stripped + NotifyConfig.normalize(env[key]?) end # A copy of the process environment, in the type the resolution works in. diff --git a/src/smith/config.cr b/src/smith/config.cr index 43dc523..aab277a 100644 --- a/src/smith/config.cr +++ b/src/smith/config.cr @@ -10,7 +10,6 @@ require "./sandbox" require "./media" require "./pricing" require "./notify_config" -require "./cmux_client" module Smith # Resolved configuration, merged from (lowest to highest priority): @@ -630,16 +629,15 @@ module Smith end # The `[notify]` section, and only that: what the config file says about - # cmux desktop notifications. The `CMUX_*` environment cmux exports into - # the processes it spawns is the other half, and merging the two is - # `Smith::CmuxClient.resolve`'s job — this deliberately does not reach for - # `ENV`, so a config file and a terminal can be reasoned about separately. + # cmux desktop notifications. # - # The result is a `NotifyConfig` rather than a nested struct here because - # `CmuxClient` already owns every rule about which value wins; keeping two - # shapes would mean keeping two sets. + # The `CMUX_*` environment the terminal exports is the other half, and this + # deliberately does not reach for it — a config file and a terminal are two + # questions, and keeping them apart is what lets either be reasoned about. + # The record does the reading: it is pure data, so no name that knows about + # sockets or variables appears here at all. def notify : NotifyConfig - CmuxClient.from_table(lookup("notify").try(&.as_h?)) + NotifyConfig.from_table(lookup("notify").try(&.as_h?)) end # Consumed by Subagents::Supervisor via CLI#build_agent. max_children = 0 diff --git a/src/smith/notify_config.cr b/src/smith/notify_config.cr index 7e66e5d..0e7a18b 100644 --- a/src/smith/notify_config.cr +++ b/src/smith/notify_config.cr @@ -1,23 +1,61 @@ +require "toml" + module Smith # Resolved configuration for cmux desktop notifications. Pure data: it holds # the effective values after config and environment have been merged, and # knows nothing about sockets or the wire protocol. # + # Reading the config tier is here for the same reason — a TOML table is data, + # and turning it into this record involves no environment, no socket and no + # protocol. Which keeps the callers of the notify subsystem down to one name: + # `Config` reads a table into this record, and everything downstream hands the + # record to `Notify`. The environment is somebody else's business. + # # Blank strings are normalised to `nil` so callers can treat "unset" and # "explicitly empty" the same way. # # `enabled` is a `Bool?` for the same reason, and it matters more here than # anywhere else in this record: `false` is somebody turning notifications # off, `nil` is nobody having said. Only the second one lets the terminal - # have a say — see `CmuxClient.resolve`. Collapsing the two would mean a - # config file that never mentions `[notify]` was read as one that refused - # it, and no amount of environment could switch them back on. + # have a say. Collapsing the two would mean a config file that never mentions + # `[notify]` was read as one that refused it, and no amount of environment + # could switch them back on. record NotifyConfig, enabled : Bool? = nil, socket_path : String? = nil, surface_id : String? = nil, workspace_id : String? = nil, - timeout : Float64 = 1.0 do + timeout : Float64 = DEFAULT_TIMEOUT do + DEFAULT_TIMEOUT = 1.0 + + # The `[notify]` section, before the environment has had a say. + # + # Blank strings become nil, so `socket_path = ""` in a config file is the + # same as the key not being there — otherwise the empty value would shadow + # the environment with nothing. A value of the wrong type is ignored rather + # than raised on, because `socket_path = true` is a typo and not a reason + # for smith to refuse to start. + def self.from_table(table : Hash(String, TOML::Any)? = nil) : NotifyConfig + timeout = float_setting(table, "timeout") + + NotifyConfig.new( + enabled: setting(table, "enabled").try(&.as_bool?), + socket_path: normalize(setting(table, "socket_path").try(&.as_s?)), + surface_id: normalize(setting(table, "surface_id").try(&.as_s?)), + workspace_id: normalize(setting(table, "workspace_id").try(&.as_s?)), + timeout: timeout.nil? || timeout <= 0 ? DEFAULT_TIMEOUT : timeout + ) + end + + # One rule for both tiers, so "unset" and "explicitly empty" cannot come + # apart between the config file and the environment: a whitespace-only + # value is an absent one, wherever it was read from. + def self.normalize(value : String?) : String? + return nil if value.nil? + stripped = value.strip + stripped.empty? ? nil : stripped + end + # True when a socket path was resolved. Without one there is no cmux # daemon to talk to, so notifications degrade to a no-op. def socket? : Bool @@ -34,5 +72,15 @@ module Smith def deliverable? : Bool enabled? && socket? end + + private def self.setting(table : Hash(String, TOML::Any)?, key : String) : TOML::Any? + table.try(&.[key]?) + end + + # TOML writes `timeout = 2` as an integer and `timeout = 0.5` as a float, + # and both are the same statement — `as_f?` reads either. + private def self.float_setting(table : Hash(String, TOML::Any)?, key : String) : Float64? + setting(table, key).try(&.as_f?) + end end end