diff --git a/agterm/AgentHooksInstaller.swift b/agterm/AgentHooksInstaller.swift index edd5eb211..061777dd4 100644 --- a/agterm/AgentHooksInstaller.swift +++ b/agterm/AgentHooksInstaller.swift @@ -131,6 +131,8 @@ enum AgentHooksInstaller { // bake the bundled agtermctl's absolute path into the installed wrappers so the hooks fire even when the // CLI was never symlinked into PATH. `[ -n "${AGTERMCTL:-}" ] ||` assigns only when unset, so an explicit // env override still wins (order 1 > 2 > PATH); shellQuote keeps spaces / metacharacters inert. + // `claudeWrapperName` is deliberately absent: the Claude adapter never calls agtermctl, it `exec`s the + // generic wrapper — which carries the baked path — so there is nothing to bake into it. private static func bakeAgtermctlPath() throws { guard let tool = bundledTool else { return } // no bundled CLI: leave the PATH fallback in place for name in [AgentHooksInstall.wrapperName, AgentHooksInstall.codexWrapperName] { diff --git a/agterm/Resources/agent-status/agterm-claude-status.sh b/agterm/Resources/agent-status/agterm-claude-status.sh new file mode 100755 index 000000000..ae1508ddf --- /dev/null +++ b/agterm/Resources/agent-status/agterm-claude-status.sh @@ -0,0 +1,59 @@ +#!/usr/bin/env bash +# Claude Code status adapter installed by agterm's Help ▸ Install Agent Status Hooks command. +# +# A worker agent spawned from inside a session (headless `claude -p` from a tool call, a second CLI +# agent) inherits the spawner's AGTERM_* environment, so its own hooks would repaint the SPAWNER's +# row. This adapter keeps that ownership question inside the installed hook package: decide from +# process topology, not terminal state. The hook is a descendant of the agent that fired it, so +# exactly one agent binary between here and the pane means "I am the pane's agent"; a second one +# means another agent spawned mine, so stay silent. A tty test cannot answer this — a headless lane +# that legitimately owns its pane has none, and a worker under script/expect gets a fresh pty anyway. +# +# It fails OPEN (reports) whenever the chain is unreadable or severed, e.g. a detached worker whose +# spawner already exited: a missed guard is the behavior without this adapter, while a false silence +# is a bug with no symptom. The argv is forwarded to the shared wrapper verbatim, so the adapter adds +# a guard and changes nothing else; it never reads stdin, so a payload another hook wants is intact. +set -u + +[ -n "${AGTERM_SESSION_ID:-}" ] || exit 0 + +script_dir=$(CDPATH= cd -- "$(dirname -- "$0")" 2>/dev/null && pwd) +status_wrapper=${AGTERM_STATUS_WRAPPER:-"$script_dir/agterm-agent-status.sh"} + +# exact names, not a glob: `claude*` also matches wrapper scripts such as claude-opus-worker. +is_agent() { + case "$1" in claude | codex | kimi | opencode | pi) return 0 ;; esac + return 1 +} + +agents=0 +p=$PPID +for _ in 1 2 3 4 5 6 7 8; do + [ -n "$p" ] && [ "$p" -gt 1 ] 2>/dev/null || break + line=$(ps -o ppid=,command= -p "$p" 2>/dev/null) || break + [ -n "$line" ] || break + line=${line#"${line%%[![:space:]]*}"} # ltrim the ppid column's padding + ppid=${line%% *} + cmd=${line#* } + + base=${cmd%% *} + base=${base##*/} # argv[0] basename + base=${base#-} # login shells report -/bin/zsh + base=${base#\(} + base=${base%\)} # macOS renders (name) when argv is unreadable + case "$base" in # node/bun-hosted CLIs present as `node .../cli.js`, so test argv[1] too + node | bun | deno | python3 | python) + rest=${cmd#* } + hosted=${rest%% *} + hosted=${hosted##*/} + is_agent "$hosted" && base=$hosted + ;; + esac + + is_agent "$base" && agents=$((agents + 1)) + [ "$agents" -gt 1 ] && exit 0 # a second agent above mine: I am a worker, not the pane's agent + case "$base" in login | agterm) break ;; esac # reached the pane boundary + p=$ppid +done + +exec "$status_wrapper" "$@" diff --git a/agtermCore/Sources/agtermCore/AgentHooksInstall.swift b/agtermCore/Sources/agtermCore/AgentHooksInstall.swift index 3dc8bbf3e..f40ba54b5 100644 --- a/agtermCore/Sources/agtermCore/AgentHooksInstall.swift +++ b/agtermCore/Sources/agtermCore/AgentHooksInstall.swift @@ -13,6 +13,12 @@ public enum AgentHooksInstall { /// terminal-output knowledge stays in this hook resource, outside agterm's runtime. public static let codexWrapperName = "agterm-codex-status.sh" + /// Claude-specific adapter the four Claude hooks invoke instead of the generic wrapper: a worker agent + /// spawned from inside a session inherits the spawner's `AGTERM_*` environment, so its hooks would repaint + /// the SPAWNER's row. The adapter answers that ownership question from process topology and delegates, + /// keeping the Claude-specific knowledge in the hook resource the way the Codex adapter does. + public static let claudeWrapperName = "agterm-claude-status.sh" + /// The bundled Pi extension's path relative to the agent-status package, and its destination filename. public static let piExtensionRelativePath = "pi/agterm-status.ts" public static let piExtensionName = "agterm-status.ts" @@ -103,14 +109,16 @@ public enum AgentHooksInstall { /// merge the four agent-status hooks into an existing Claude Code `settings.json`. /// /// `existing` is the current contents (nil/empty = start from a fresh object). Returns the new JSON and - /// whether it differs; idempotent — hooks already present (detected by the wrapper command) return the - /// input with `changed == false`. Unrelated hooks and keys are preserved; invalid JSON throws. + /// whether it differs; idempotent — hooks already present (detected by the adapter command) return the + /// input with `changed == false`. Unrelated hooks and keys are preserved; invalid JSON throws. Entries a + /// PRIOR install pointed straight at the generic wrapper are migrated onto the Claude adapter first, so + /// the ownership guard reaches an existing install rather than only a fresh one. public static func mergeClaudeSettings(existing: String?, scriptDir: String) throws -> (json: String, changed: Bool) { let command = wrapperCommand(scriptDir: scriptDir) var root = try parsedObject(existing) var hooks = root["hooks"] as? [String: Any] ?? [:] - var didChange = false + var didChange = migrateClaudeEntriesToAdapter(&hooks, scriptDir: scriptDir) for hook in claudeHooks { var entries = hooks[hook.event] as? [[String: Any]] ?? [] if entries.contains(where: { entryUsesWrapper($0, scriptDir: scriptDir) }) { @@ -294,6 +302,10 @@ public enum AgentHooksInstall { scriptDir + "/" + codexWrapperName } + public static func claudeWrapperPath(scriptDir: String) -> String { + scriptDir + "/" + claudeWrapperName + } + /// render the `~/.codex/config.toml` `[[hooks.*]]` block the installer merges in, wiring Codex's lifecycle /// events to the indicator. `site/docs.html#codex-hooks-manual` reproduces this block for the cases the /// merge declines, and nothing checks the two against each other. @@ -328,9 +340,47 @@ public enum AgentHooksInstall { } } - // build the command string a Claude hook runs: the quoted wrapper path plus the state argument. + // build the command string a Claude hook runs: the quoted CLAUDE ADAPTER path plus the state argument. The + // adapter forwards the state to the generic wrapper verbatim once it decides the firing agent owns the pane. private static func wrapperCommand(scriptDir: String) -> String { - shellQuote(wrapperPath(scriptDir: scriptDir)) + " " + shellQuote(claudeWrapperPath(scriptDir: scriptDir)) + " " + } + + // repoint an EARLIER install's four entries from the generic wrapper at the Claude adapter, returning + // whether anything moved. Without it the guard would reach fresh installs only: the merge below skips an + // event whose entries already invoke us, and the Claude side has no refresh path (`refreshManagedCodexBlock` + // is Codex-only), so an existing settings.json would keep its unguarded entries forever. + // + // The match is BYTE-EXACT against the command this installer generates for that same event — the quoted + // wrapper path plus the state — which is what makes the rewrite safe: a hand-edited entry, an entry + // carrying extra flags, and a user's own hook that merely mentions the wrapper all fail the comparison and + // are left alone, and only the command string is replaced, so a matcher and any sibling keys survive. + // Idempotent, because a migrated entry names the adapter and no longer matches. + private static func migrateClaudeEntriesToAdapter(_ hooks: inout [String: Any], scriptDir: String) -> Bool { + let generated = shellQuote(wrapperPath(scriptDir: scriptDir)) + " " + let replacement = wrapperCommand(scriptDir: scriptDir) + var didChange = false + for hook in claudeHooks { + guard var entries = hooks[hook.event] as? [[String: Any]] else { continue } + var eventChanged = false + for index in entries.indices { + guard var commands = entries[index]["hooks"] as? [[String: Any]] else { continue } + var entryChanged = false + for slot in commands.indices where commands[slot]["command"] as? String == generated + hook.state { + commands[slot]["command"] = replacement + hook.state + entryChanged = true + } + if entryChanged { + entries[index]["hooks"] = commands + eventChanged = true + } + } + if eventChanged { + hooks[hook.event] = entries + didChange = true + } + } + return didChange } // a single Claude hook entry: { (matcher?), hooks: [{ type: command, command }] }. @@ -344,11 +394,19 @@ public enum AgentHooksInstall { return entry } - // does a hook entry already invoke our wrapper (idempotency probe, by wrapper path)? + // does a hook entry already invoke us (idempotency probe)? EITHER path counts: the adapter, which is what + // a current install writes and what the migration leaves behind, and the generic wrapper, which is what an + // entry the migration declined to rewrite still names. Accepting only the adapter would answer "not + // installed" for a customized wrapper entry and add a stock one beside it, so both would fire and the row + // would be posted twice. Its owner keeps the setup they edited, unguarded by their own choice — the same + // answer this probe has always given. private static func entryUsesWrapper(_ entry: [String: Any], scriptDir: String) -> Bool { - let probe = wrapperPath(scriptDir: scriptDir) + let probes = [claudeWrapperPath(scriptDir: scriptDir), wrapperPath(scriptDir: scriptDir)] guard let commands = entry["hooks"] as? [[String: Any]] else { return false } - return commands.contains { ($0["command"] as? String)?.contains(probe) == true } + return commands.contains { command in + guard let command = command["command"] as? String else { return false } + return probes.contains { command.contains($0) } + } } // absent/empty/whitespace-only → fresh empty object; a non-empty file that is not a valid JSON object → diff --git a/agtermCore/Tests/agtermCoreTests/AgentHooksInstallTests.swift b/agtermCore/Tests/agtermCoreTests/AgentHooksInstallTests.swift index eb3f12ad4..cf63967ea 100644 --- a/agtermCore/Tests/agtermCoreTests/AgentHooksInstallTests.swift +++ b/agtermCore/Tests/agtermCoreTests/AgentHooksInstallTests.swift @@ -31,12 +31,14 @@ struct AgentHooksInstallTests { #expect(evts["PostToolUse"]?.count == 1) #expect(evts["Stop"]?.count == 1) #expect(evts["Notification"]?.count == 1) - #expect(command(evts["UserPromptSubmit"]![0])?.hasSuffix("agent-status.sh' active --blink") == true) + // the entries invoke the Claude adapter, which guards on ownership and forwards to the wrapper + let adapter = AgentHooksInstall.claudeWrapperPath(scriptDir: scriptDir) + #expect(command(evts["UserPromptSubmit"]![0]) == "'\(adapter)' active --blink") // PostToolUse re-asserts active after every tool, clearing a lingering blocked on resume - #expect(command(evts["PostToolUse"]![0])?.hasSuffix("agent-status.sh' active --blink") == true) + #expect(command(evts["PostToolUse"]![0]) == "'\(adapter)' active --blink") // only the Stop hook passes --auto-reset (clear-on-visit); active/blocked stay keep-state - #expect(command(evts["Stop"]![0])?.hasSuffix("agent-status.sh' completed --auto-reset") == true) - #expect(command(evts["Notification"]![0])?.hasSuffix("agent-status.sh' blocked") == true) + #expect(command(evts["Stop"]![0]) == "'\(adapter)' completed --auto-reset") + #expect(command(evts["Notification"]![0]) == "'\(adapter)' blocked") #expect(command(evts["UserPromptSubmit"]![0])?.contains("--auto-reset") == false) #expect(command(evts["Notification"]![0])?.contains("--auto-reset") == false) #expect(evts["Notification"]![0]["matcher"] as? String == "permission_prompt") @@ -75,7 +77,7 @@ struct AgentHooksInstallTests { #expect(evts["UserPromptSubmit"]?.count == 2) let commands = evts["UserPromptSubmit"]!.compactMap { command($0) } #expect(commands.contains("/usr/bin/other-hook.sh")) - #expect(commands.contains { $0.hasSuffix("agent-status.sh' active --blink") }) + #expect(commands.contains { $0.hasSuffix("claude-status.sh' active --blink") }) #expect(evts["PostToolUse"]?.count == 1) #expect(evts["Stop"]?.count == 1) #expect(evts["Notification"]?.count == 1) @@ -92,6 +94,110 @@ struct AgentHooksInstallTests { #expect(commands.contains("/usr/bin/other.sh")) } + // settings.json exactly as an install BEFORE the Claude adapter left it: four entries invoking the + // generic wrapper directly. + private func legacySettings(extraUserHook: String? = nil) -> String { + let wrapper = AgentHooksInstall.wrapperPath(scriptDir: scriptDir) + var userHook = "" + if let extraUserHook { + userHook = """ + , + {"hooks": [{"type": "command", "command": "\(extraUserHook)"}]} + """ + } + return """ + { + "hooks": { + "UserPromptSubmit": [ + {"hooks": [{"type": "command", "command": "'\(wrapper)' active --blink"}]}\(userHook) + ], + "PostToolUse": [ + {"hooks": [{"type": "command", "command": "'\(wrapper)' active --blink"}]} + ], + "Stop": [ + {"hooks": [{"type": "command", "command": "'\(wrapper)' completed --auto-reset"}]} + ], + "Notification": [ + {"matcher": "permission_prompt", "hooks": [{"type": "command", "command": "'\(wrapper)' blocked"}]} + ] + } + } + """ + } + + @Test func mergeMigratesEarlierInstallOntoTheAdapter() throws { + // the guard has to reach an EXISTING install: without the migration the merge would skip all four + // events as already-installed and leave them pointing at the unguarded wrapper forever + let result = try AgentHooksInstall.mergeClaudeSettings(existing: legacySettings(), scriptDir: scriptDir) + #expect(result.changed) + let evts = events(result.json) + let adapter = AgentHooksInstall.claudeWrapperPath(scriptDir: scriptDir) + #expect(evts["UserPromptSubmit"]?.count == 1) + #expect(evts["PostToolUse"]?.count == 1) + #expect(evts["Stop"]?.count == 1) + #expect(evts["Notification"]?.count == 1) + #expect(command(evts["UserPromptSubmit"]![0]) == "'\(adapter)' active --blink") + #expect(command(evts["PostToolUse"]![0]) == "'\(adapter)' active --blink") + #expect(command(evts["Stop"]![0]) == "'\(adapter)' completed --auto-reset") + #expect(command(evts["Notification"]![0]) == "'\(adapter)' blocked") + // rewritten in place: the matcher and the entry's other keys are untouched + #expect(evts["Notification"]![0]["matcher"] as? String == "permission_prompt") + #expect((evts["Stop"]![0]["hooks"] as? [[String: Any]])?.first?["type"] as? String == "command") + // migrated, not appended alongside a fresh set + #expect(!result.json.contains(AgentHooksInstall.wrapperPath(scriptDir: scriptDir) + "'")) + } + + @Test func mergeMigrationIsIdempotent() throws { + let first = try AgentHooksInstall.mergeClaudeSettings(existing: legacySettings(), scriptDir: scriptDir) + let second = try AgentHooksInstall.mergeClaudeSettings(existing: first.json, scriptDir: scriptDir) + #expect(!second.changed) + #expect(second.json == first.json) + } + + @Test func mergeMigrationLeavesCustomizedEntryAlone() throws { + // one hand-edited entry (an appended flag) plus one entry still in generated form: byte-exactness is + // the whole safety property, so the edited one must come back identical + let wrapper = AgentHooksInstall.wrapperPath(scriptDir: scriptDir) + let customized = "'\(wrapper)' active --blink --pane right" + let existing = """ + { + "hooks": { + "UserPromptSubmit": [ + {"hooks": [{"type": "command", "command": "\(customized)"}]} + ], + "Stop": [ + {"hooks": [{"type": "command", "command": "'\(wrapper)' completed --auto-reset"}]} + ] + } + } + """ + let result = try AgentHooksInstall.mergeClaudeSettings(existing: existing, scriptDir: scriptDir) + #expect(result.changed) + let evts = events(result.json) + let adapter = AgentHooksInstall.claudeWrapperPath(scriptDir: scriptDir) + // the customized entry survives byte-identical AND still counts as installed, so no stock entry is + // added beside it: two entries would both fire and post the row twice + #expect(evts["UserPromptSubmit"]?.count == 1) + #expect(evts["UserPromptSubmit"]!.compactMap { command($0) } == [customized]) + // the entry that WAS in generated form migrated in place rather than gaining a duplicate + #expect(evts["Stop"]?.count == 1) + #expect(command(evts["Stop"]![0]) == "'\(adapter)' completed --auto-reset") + } + + @Test func mergeMigrationLeavesUserHookNamingTheWrapperAlone() throws { + // a user's own hook that merely mentions the wrapper path is not something the installer wrote + let wrapper = AgentHooksInstall.wrapperPath(scriptDir: scriptDir) + let userHook = "my-notifier.sh && '\(wrapper)' active --blink" + let result = try AgentHooksInstall.mergeClaudeSettings(existing: legacySettings(extraUserHook: userHook), + scriptDir: scriptDir) + #expect(result.changed) + let prompts = events(result.json)["UserPromptSubmit"]!.compactMap { command($0) } + #expect(prompts.contains(userHook)) + // only the generated sibling moved onto the adapter + #expect(prompts.contains("'\(AgentHooksInstall.claudeWrapperPath(scriptDir: scriptDir))' active --blink")) + #expect(prompts.count == 2) + } + @Test func mergeRefusesMalformedExisting() { // refusing leaves the user's hand-maintained settings.json untouched #expect(throws: AgentHooksInstall.MergeError.self) { diff --git a/agtermCore/Tests/agtermCoreTests/ClaudeStatusHookTests.swift b/agtermCore/Tests/agtermCoreTests/ClaudeStatusHookTests.swift new file mode 100644 index 000000000..327d30413 --- /dev/null +++ b/agtermCore/Tests/agtermCoreTests/ClaudeStatusHookTests.swift @@ -0,0 +1,122 @@ +import Foundation +import Testing + +// Exercises only the Claude adapter shipped by the Help-menu installer. Its guard is a claim about process +// TOPOLOGY, so each case builds a real chain of processes above the script rather than stubbing one out. +struct ClaudeStatusHookTests { + private static var hook: String { + URL(fileURLWithPath: #filePath) + .deletingLastPathComponent() + .deletingLastPathComponent() + .deletingLastPathComponent() + .deletingLastPathComponent() + .appendingPathComponent("agterm/Resources/agent-status/agterm-claude-status.sh") + .path + } + + // spawns `chain` as nested processes — outermost first, one process per entry, each carrying that entry + // as its argv[0] — and runs the adapter under the innermost one with `args`. `host:agent` builds a + // runtime-hosted process instead: argv[0] is the host, argv[1] a script named after the agent. + // + // Every chain names its own boundary, because the adapter stops walking at `login`. Without one the test + // would inherit whatever ancestry the runner happens to have, and `swift test` run from inside a Claude + // session — the very thing this adapter exists for — would put a real `claude` above the fixture and turn + // the owner cases silent. The boundary makes each case depend only on the processes it builds. + private func run(chain: [String], args: [String], sessionID: String? = "sid") throws -> (calls: [String], exit: Int32) { + let fm = FileManager.default + let dir = URL(fileURLWithPath: NSTemporaryDirectory()).appendingPathComponent("agterm-claude-hook-\(UUID().uuidString)") + try fm.createDirectory(at: dir, withIntermediateDirectories: true) + defer { try? fm.removeItem(at: dir) } + + let calls = dir.appendingPathComponent("calls") + let wrapper = dir.appendingPathComponent("status-wrapper") + let spawner = dir.appendingPathComponent("spawn-chain.sh") + try "#!/bin/bash\nprintf '%s\\n' \"$*\" >> '\(calls.path)'\n".write(to: wrapper, atomically: true, encoding: .utf8) + // each level re-enters this script with one fewer name. The subshell is what makes the level a + // separate process: `exec -a` replaces its own process, so without the fork the whole chain would + // collapse onto one pid, and the trailing `exit` keeps bash from optimizing that fork away. + try """ + #!/bin/bash + set -u + dir=$(cd "$(dirname "$0")" && pwd) + levels=() + while [ "$#" -gt 0 ] && [ "$1" != "--" ]; do levels+=("$1"); shift; done + shift + if [ "${#levels[@]}" -eq 0 ]; then + "$@" + exit $? + fi + first=${levels[0]} + rest=("${levels[@]:1}") + case "$first" in + *:*) ( exec -a "${first%%:*}" /bin/bash "$dir/${first#*:}" ${rest[@]+"${rest[@]}"} -- "$@" ) ;; + *) ( exec -a "$first" /bin/bash "$0" ${rest[@]+"${rest[@]}"} -- "$@" ) ;; + esac + exit $? + """.write(to: spawner, atomically: true, encoding: .utf8) + try fm.setAttributes([.posixPermissions: 0o755], ofItemAtPath: wrapper.path) + try fm.setAttributes([.posixPermissions: 0o755], ofItemAtPath: spawner.path) + // a hosted level needs the spawner under the agent's own name, so the walk reads it as argv[1] + for level in chain where level.contains(":") { + try fm.copyItem(at: spawner, to: dir.appendingPathComponent(String(level.split(separator: ":")[1]))) + } + + let proc = Process() + proc.executableURL = URL(fileURLWithPath: "/bin/bash") + proc.arguments = [spawner.path] + chain + ["--", Self.hook] + args + var environment = [ + "AGTERM_STATUS_WRAPPER": wrapper.path, + "AGTERM_SOCKET": "/tmp/agterm.sock", + "PATH": "/usr/bin:/bin", + ] + environment["AGTERM_SESSION_ID"] = sessionID + proc.environment = environment + proc.standardOutput = Pipe() + proc.standardError = Pipe() + try proc.run() + proc.waitUntilExit() + + let recorded = ((try? String(contentsOf: calls, encoding: .utf8)) ?? "") + .split(separator: "\n").map(String.init) + return (recorded, proc.terminationStatus) + } + + @Test func paneAgentDelegatesWithArgvVerbatim() throws { + // one agent between the hook and the pane: the firing agent owns the row, so the call goes through + // with every argument intact — the flags are the wrapper's, and a user may have appended more + let result = try run(chain: ["login", "claude"], args: ["completed", "--auto-reset", "--pane", "right"]) + #expect(result.calls == ["completed --auto-reset --pane right"]) + #expect(result.exit == 0) + } + + @Test func spawnedWorkerStaysSilent() throws { + // a second agent above ours means another agent spawned this one, so its status belongs to a session + // that is not the pane's — the row it would repaint is the SPAWNER's + let result = try run(chain: ["login", "claude", "claude"], args: ["completed", "--auto-reset"]) + #expect(result.calls.isEmpty) + #expect(result.exit == 0) + } + + @Test func runtimeHostedAgentIsCountedThroughArgv1() throws { + // a node/bun-hosted CLI presents as `node …/claude`, so argv[0] alone would miss it and the worker + // would report. Counting it makes this the same two-agent chain as above. + let result = try run(chain: ["login", "claude", "node:claude"], args: ["active", "--blink"]) + #expect(result.calls.isEmpty) + #expect(result.exit == 0) + } + + @Test func walkStopsAtThePaneBoundary() throws { + // an agent BEYOND the pane boundary is not this pane's business: the walk stops at `login`, so the + // outer claude is never counted and the inner one still reads as the pane's own agent + let result = try run(chain: ["claude", "login", "claude"], args: ["active", "--blink"]) + #expect(result.calls == ["active --blink"]) + #expect(result.exit == 0) + } + + @Test func outsideAgtermExitsSilently() throws { + // no session id: nothing to address, and a hook must never fail the turn it fired from + let result = try run(chain: ["login", "claude"], args: ["completed", "--auto-reset"], sessionID: nil) + #expect(result.calls.isEmpty) + #expect(result.exit == 0) + } +}