From 05c14b7ac35cf00003c50ec8fb2d509f6716e89d Mon Sep 17 00:00:00 2001 From: m1kekad0 Date: Wed, 19 Aug 2026 20:24:45 +0900 Subject: [PATCH 01/34] feat: add Devin agent type AI-assisted-by: Gemini 3.7 Flash --- README.ja.md | 2 +- README.md | 2 +- docs/agent-types.md | 2 +- install.sh | 4 +- scripts/drivers/types/devin/_delivery.sh | 21 +++ scripts/drivers/types/devin/template.md | 229 +++++++++++++++++++++++ scripts/drivers/types/devin/type.conf | 7 + site/src/i18n/de.js | 3 +- site/src/i18n/en.js | 1 + site/src/i18n/es.js | 5 +- site/src/i18n/fr.js | 1 + site/src/i18n/ja.js | 1 + site/src/i18n/ko.js | 1 + site/src/i18n/pt-BR.js | 5 +- site/src/i18n/zh-CN.js | 1 + site/src/i18n/zh-TW.js | 5 +- site/src/layouts/Home.astro | 1 + tests/test_delivery.bats | 32 ++++ tests/test_type_registry.bats | 20 +- 19 files changed, 321 insertions(+), 22 deletions(-) create mode 100644 scripts/drivers/types/devin/_delivery.sh create mode 100644 scripts/drivers/types/devin/template.md create mode 100644 scripts/drivers/types/devin/type.conf diff --git a/README.ja.md b/README.ja.md index 2bb4e11b9..678ba90cf 100644 --- a/README.ja.md +++ b/README.ja.md @@ -210,7 +210,7 @@ codex: --dangerously-skip-permissions: false # `false`の値はフラグ自体を出力しない ``` -9種類のエージェントタイプのうち8つがspawn可能 — `claude-code`、`codex`、`grok-build`、`cursor`、`gemini`、`antigravity`、`copilot`、`opencode`。`hermes` は不可 — そのCLIには初期プロンプトを事前に仕込んだインタラクティブセッションを開始するモードがない（#279）。macOSが主なターゲットで、LinuxとWindowsはベストエフォート（ターミナルが未対応の場合はissueまたはPRを歓迎）。ヘッドレス環境 — tmuxもなく使えるターミナルもない — はエージェントCLIがインタラクティブなターミナルを必要とするためエラーになる。 +10種類のエージェントタイプのうち8つがspawn可能 — `claude-code`、`codex`、`grok-build`、`cursor`、`gemini`、`antigravity`、`copilot`、`opencode`。`hermes` と `devin` は不可 — 初期プロンプトを事前に仕込んだインタラクティブセッションを開始するモードがない。macOSが主なターゲットで、LinuxとWindowsはベストエフォート（ターミナルが未対応の場合はissueまたはPRを歓迎）。ヘッドレス環境 — tmuxもなく使えるターミナルもない — はエージェントCLIがインタラクティブなターミナルを必要とするためエラーになる。 ### spawnしたエージェントを終了する（`despawn`） diff --git a/README.md b/README.md index b775aedf9..a6ccdfd3a 100644 --- a/README.md +++ b/README.md @@ -211,7 +211,7 @@ codex: --dangerously-skip-permissions: false # a `false` value suppresses the flag entirely ``` -Eight of the nine agent types are spawnable — `claude-code`, `codex`, `grok-build`, `cursor`, `gemini`, `antigravity`, `copilot`, `opencode`. `hermes` is not: its CLI has no mode that starts an interactive session pre-seeded with an initial prompt (#279). macOS is the primary target; Linux and Windows are best-effort (please open an issue/PR if your terminal isn't handled). Headless environments — no tmux **and** no usable terminal — error out, since the agent CLIs need an interactive terminal. +Eight of the ten agent types are spawnable — `claude-code`, `codex`, `grok-build`, `cursor`, `gemini`, `antigravity`, `copilot`, `opencode`. `hermes` and `devin` are not: their CLIs do not have a known interactive session boot mode pre-seeded with an initial prompt. macOS is the primary target; Linux and Windows are best-effort (please open an issue/PR if your terminal isn't handled). Headless environments — no tmux **and** no usable terminal — error out, since the agent CLIs need an interactive terminal. ### Tear down a spawned agent (`despawn`) diff --git a/docs/agent-types.md b/docs/agent-types.md index ea84c06c7..ef1a17a61 100644 --- a/docs/agent-types.md +++ b/docs/agent-types.md @@ -1,7 +1,7 @@ # Agent types agmsg supports several agent runtimes — claude-code, codex, gemini, antigravity, -copilot, opencode, hermes, cursor — and each is described by a small **manifest** so that the rest +copilot, opencode, hermes, cursor, devin, grok-build — and each is described by a small **manifest** so that the rest of agmsg (detection, the join whitelist, spawn, and delivery routing) discovers it from data instead of hardcoded `case` arms. diff --git a/install.sh b/install.sh index afa0570ff..ccf4507c8 100755 --- a/install.sh +++ b/install.sh @@ -103,7 +103,7 @@ AGENT_TYPE="" # claude-code, codex, gemini, antigravity — passed via --agent- # cursor) that had already drifted from the other two -- re-detecting one of # those three types as "codex" and then, via the template pick, overwriting # the SKILL.md the installer itself had written with the wrong flavor. -AGMSG_SHARED_SKILL_TPL_TYPES="gemini antigravity opencode hermes cursor grok-build" +AGMSG_SHARED_SKILL_TPL_TYPES="gemini antigravity opencode hermes cursor grok-build devin" configure_codex_sandbox() { # --- Configure Codex sandbox (if Codex is installed) --- @@ -219,7 +219,7 @@ while [[ $# -gt 0 ]]; do echo "Options:" echo " --cmd Command & skill folder name (default: agmsg)" echo " Claude Code: /, Codex/Gemini/Antigravity: \$" - echo " --agent-type Agent type: claude-code, codex, gemini, antigravity, opencode, hermes, cursor, grok-build" + echo " --agent-type Agent type: claude-code, codex, gemini, antigravity, opencode, hermes, cursor, grok-build, devin" echo " Selects which template becomes SKILL.md (matches the" echo " arg passed to join.sh / whoami.sh)" echo " --update Update skill scripts only (preserve DB and teams)" diff --git a/scripts/drivers/types/devin/_delivery.sh b/scripts/drivers/types/devin/_delivery.sh new file mode 100644 index 000000000..4cc7d5ebe --- /dev/null +++ b/scripts/drivers/types/devin/_delivery.sh @@ -0,0 +1,21 @@ +#!/usr/bin/env bash +# devin delivery plug — manual inbox checks only. +# +# Devin has no agmsg automatic delivery hook: the only valid mode is `off` +# (enforced by delivery_modes=off in the manifest, which the central gate in +# delivery.sh checks). These overrides keep apply / status / teardown from +# touching a hook file or another agent type's watchers. Sourced into +# delivery.sh's context. + +# Nothing to write — manual-only, so no hooks_file is resolved or created. +agmsg_delivery_apply() { :; } + +# No hook file to read; the mode is always off. +agmsg_delivery_status() { echo "mode: off"; } + +# No watcher or bridge of our own. Do NOT fall through to the default teardown +# (which stops this project's watch.sh) — another agent type may hold a live +# watcher on the same project, and a devin `set off` must not disturb it. +agmsg_delivery_on_disable() { + echo "Devin has no agmsg automatic delivery hook; manual inbox checks only." +} diff --git a/scripts/drivers/types/devin/template.md b/scripts/drivers/types/devin/template.md new file mode 100644 index 000000000..aa1a61fa6 --- /dev/null +++ b/scripts/drivers/types/devin/template.md @@ -0,0 +1,229 @@ +--- +name: __SKILL_NAME__ +description: Cross-agent messaging via SQLite. Send messages between Claude Code, Codex, Gemini CLI, Devin, and other agents. No daemon, no network, no dependencies beyond bash and sqlite3. +--- + +Devin skill for agmsg cross-agent messaging. **IMPORTANT: Always use the provided scripts. NEVER directly read or edit config files, DB, or team data. There is NO register.sh — use join.sh to join a team.** + +**Shell requirement:** All agmsg scripts are Bash scripts. Always execute them via `bash`, never via PowerShell or cmd directly. If your default shell is not Bash (e.g. PowerShell on Windows), wrap every command with `bash -lc '...'`. Example: `bash -lc '~/.agents/skills/__SKILL_NAME__/scripts/send.sh myteam alice bob "hello"'`. Do NOT construct DB paths manually — the scripts handle path resolution internally. If you need to redirect storage, use `AGMSG_STORAGE_PATH` (the supported override). + +## Identity + +If you already know your AGENT and TEAMS from a previous `/__SKILL_NAME__` use in this session, skip to **Execute** below. + +Otherwise, run: `~/.agents/skills/__SKILL_NAME__/scripts/whoami.sh "$(pwd)" devin` + +Four possible outputs: + +**A) Single identity:** +`agent= teams= type=devin project=` +→ Remember AGENT and TEAMS, then go to **Execute**. + +**B) Multiple identities:** +`multiple=true agents= teams= type=devin project=` +→ Ask the user which agent name to use for this session, then go to **Execute**. + +**C) Not in a team:** +`not_joined=true available_teams=` (or `available_teams=none`) +→ Show the user the available teams from the output, then: + + Before first-time setup, inspect the user's request. If they ask to join, import, or bring in a team that already exists on a server, do not call `join.sh`. Go directly to `remote pull` under Execute. First run `~/.agents/skills/__SKILL_NAME__/scripts/team-list.sh --json --scope all`; if a same-named local team has `binding_state` `none` or `disconnected`, stop and ask the user how to proceed. After pull succeeds, return to Identity setup so the user can register a new local agent in the pulled team. + + > **First-time setup required.** + > Joining a team so this agent can send and receive messages. + > - **Team name**: a group of agents that can message each other (available: ) + > - **Agent name**: this agent's identity within the team + + 1. Ask: "Enter a team name (joins existing or creates new)" + 2. If the team name given already appears in `available_teams`, run `~/.agents/skills/__SKILL_NAME__/scripts/team.sh ` to see the current roster (name, type, project) and note the names already in use. Look for a naming convention already in play (e.g. a shared base name with role and number suffixes (`-`), or names derived from the team name) and, when one exists, propose 2-3 unused names that extend it; otherwise propose 2-3 short, distinctive identity names (not a bare tool-type label like `codex`/`cc`). Either way, names must not collide with the roster. Then ask: "Enter a name for this agent (suggestions: , , — or type your own)". For a brand-new team, skip the roster check and just ask: "Enter a name for this agent". + 3. **You MUST use join.sh** — run: `~/.agents/skills/__SKILL_NAME__/scripts/join.sh devin "$(pwd)"` + 4. Show the result and explain: + + > **Joined!** You can now use `/__SKILL_NAME__` to check and send messages. + > - ask to check inbox — check unread messages + > - ask `send ` — send a message + > - ask `team` — list team members + > - ask `history` — message history + + 5. Devin has no agmsg automatic delivery hook. Set manual delivery explicitly: + `~/.agents/skills/__SKILL_NAME__/scripts/delivery.sh set off devin "$(pwd)"` + + 6. Then check inbox for the newly joined team. + +**D) Suggestions for reuse:** +`suggest=true agents= teams= type=devin project= available_teams=` +→ No exact registration exists for this project, but there are same-type agent names registered elsewhere. + + 1. Show the suggested agent names to the user. + 2. Ask whether to reuse one of those names or choose a new one. + 3. Ask for the team name to join (existing or new). + 4. Run: `~/.agents/skills/__SKILL_NAME__/scripts/join.sh devin "$(pwd)"` + 5. Then continue with the normal post-join flow above. + +## Execute + +**Only use scripts in `~/.agents/skills/__SKILL_NAME__/scripts/` — do not read or modify files under `teams/` or `db` directly.** + +**If no arguments provided (DEFAULT action — always do this when the command is invoked without arguments):** +1. **IMMEDIATELY** run inbox check for each TEAM: `~/.agents/skills/__SKILL_NAME__/scripts/inbox.sh $TEAM $AGENT` +2. Do NOT ask the user what to do — just run the inbox check. +3. If there are messages, read and respond appropriately. To reply: + `~/.agents/skills/__SKILL_NAME__/scripts/send.sh $TEAM $AGENT ""` + +If argument is "history": +1. Run: `~/.agents/skills/__SKILL_NAME__/scripts/history.sh $TEAM $AGENT` + +If argument starts with "team list" (e.g. "team list", "team list --json", "team list --scope project"): +1. Run: `~/.agents/skills/__SKILL_NAME__/scripts/team-list.sh ` +2. This is a distinct command from bare "team" below — check for "team list" FIRST so "list" is never mistaken for a team name. + +If argument is "team": +1. For each TEAM, run: `~/.agents/skills/__SKILL_NAME__/scripts/team.sh $TEAM` + +If argument starts with "send" (e.g. "send misaki check the server"): +1. Parse target agent and message from the arguments +2. Determine which team the target agent belongs to, then run: + `~/.agents/skills/__SKILL_NAME__/scripts/send.sh $TEAM $AGENT ""` + +If argument is "config": +1. Run: `~/.agents/skills/__SKILL_NAME__/scripts/config.sh show` +2. Show the output to the user. + +If argument starts with "config set" (e.g. "config set hook.check_interval 30"): +1. Parse key and value from the arguments. +2. Run: `~/.agents/skills/__SKILL_NAME__/scripts/config.sh set ` + +If argument starts with "actas" followed by an agent name (e.g. "actas alice"): +1. Parse the new role name. If none was given (e.g. bare "actas", or the user asks you to suggest one), run `~/.agents/skills/__SKILL_NAME__/scripts/team.sh ` for each TEAM to see the current roster. Look for a naming convention already in play (e.g. a shared base name with role and number suffixes (`-`), or names derived from the team name) and, when one exists, propose 2-3 unused names that extend it; otherwise propose 2-3 short, distinctive identity names (not a bare tool-type label). Either way, names must not collide with the roster. Ask the user to pick one or type their own before continuing. +2. Run `~/.agents/skills/__SKILL_NAME__/scripts/identities.sh "$(pwd)" devin` to see whether the role is already registered for this (project, type). +3. If the name does not appear in the output, join under the existing team. For a single team, run `~/.agents/skills/__SKILL_NAME__/scripts/join.sh devin "$(pwd)"`. For multiple teams, ask the user which team to join the new role into. +4. Set the session's active FROM to `` for every `send.sh` call until another `actas`. +5. Tell the user: "Now acting as ``. Sends will use `` as the from agent." + +If argument starts with "drop" followed by an agent name (e.g. "drop alice"): +1. Parse the role name. +2. Run `~/.agents/skills/__SKILL_NAME__/scripts/reset.sh "$(pwd)" devin ` to remove that role's registration. +3. If the session's active FROM was ``, clear that state. +4. Tell the user: "Dropped role `` from this project." + +If argument is "mode" (no further args): +1. Run: `~/.agents/skills/__SKILL_NAME__/scripts/delivery.sh status devin "$(pwd)"` +2. Show the output to the user. + +If argument starts with "mode" followed by a mode name (e.g. "mode off"): +1. Parse the mode. Devin supports only `off`. +2. If the requested mode is `off`, run: `~/.agents/skills/__SKILL_NAME__/scripts/delivery.sh set off devin "$(pwd)"` +3. If the requested mode is `monitor`, `both`, or `turn`, do not run a command; tell the user: "Devin has no agmsg automatic delivery hook; only `off` mode is supported." + +If argument is "hook on" (legacy alias): +1. Tell the user: "Devin has no agmsg automatic delivery hook; use manual inbox checks." + +If argument is "hook off" (legacy alias): +1. Run: `~/.agents/skills/__SKILL_NAME__/scripts/delivery.sh set off devin "$(pwd)"` +2. Tell the user: "Delivery mode set to 'off'." + +If argument is "version": +1. Run: `~/.agents/skills/__SKILL_NAME__/scripts/version.sh` +2. Show the output — the installed version (git-describe provenance recorded at install time). + +If argument is "reset": +1. Run: `~/.agents/skills/__SKILL_NAME__/scripts/reset.sh "$(pwd)" devin` +2. Tell the user the result. + +If argument starts with "rename" but not "rename-team": +1. Accept only an explicit user request. Parse either ` `, or ` ` only when this agent belongs to exactly one team. +2. Never invent either name. Before execution, repeat the resolved team, old name, and new name and ask the user to confirm. Wait for confirmation. +3. Run: `bash ~/.agents/skills/__SKILL_NAME__/scripts/rename.sh ` +4. Show the result. For a connected team, the `member_renamed` journal event propagates the rename to other machines. + +If argument starts with "rename-team": +1. Accept only an explicit user request. Parse ` `. +2. Never invent either team name. Before execution, repeat the old and new team names and ask the user to confirm. Wait for confirmation. +3. Run: `bash ~/.agents/skills/__SKILL_NAME__/scripts/rename-team.sh ` +4. Show the result. + +If argument starts with "remote connect": +1. Parse the required `--endpoint ` and ``, plus optional `--e2ee`. +2. Run: `bash ~/.agents/skills/__SKILL_NAME__/scripts/remote.sh connect --endpoint [--e2ee] ` +3. Show the output to the user. Plain sync is the default; pass `--e2ee` only when the user explicitly requests end-to-end encryption. The choice is fixed by the first connect. +4. End by showing this copy-paste command for the other machine, with the actual endpoint and team substituted: `bash ~/.agents/skills/__SKILL_NAME__/scripts/remote.sh pull --endpoint ` + +If argument starts with "remote pull": +1. When the user asks to join or bring in a team that already exists on a server, NEVER use `join.sh`, create a team, or create a same-named local team. Always use remote pull. +2. Before pulling, check for a same-named local team. If one already exists without an active remote connection, stop and ask the user how to proceed; do not overwrite, merge, connect, or rename it on your own. +3. Parse the required `--endpoint ` and ``, plus optional `--team-id `. +4. Run: `bash ~/.agents/skills/__SKILL_NAME__/scripts/remote.sh pull --endpoint [--team-id ] ` +5. Show the output to the user. + +Machine B needs its own install, not just its own environment variables. +Only `remote.sh`, `remote-sync.sh`, `key.sh` and the two internal helpers read +`AGMSG_SYNC_CONNECTION_DIR`; `send.sh`, `history.sh`, `team.sh` and `inbox.sh` +resolve the team config from the install directory. So a pull driven by +environment variables alone succeeds, and the send that is supposed to confirm +it then reports the team as missing — the failure lands one step after the +cause. See "Use a separate install for testing" in `docs/remote-setup.md`. + +**What e2ee changes, and what it doesn't.** The local store stays plaintext either way — `history`, `inbox`, and `send` read and write exactly the same regardless of a team's encryption setting. Only the SERVER side differs: an e2ee team's server rows carry `cipher: age-v1` and hold sealed ciphertext, so `from`, `to`, and `body` are not readable there; a plain team's rows are not sealed. Keys never pass through the server — moving one to another machine means carrying a handoff bundle by hand (`key handoff` above). + +**Readable local history is therefore not evidence that a team is unencrypted.** To state whether a given team is e2ee, ask the program — `remote status ` below — never infer it from what you can read locally. + +If argument starts with "remote unlock": +1. Parse ``, `--bundle `, and `--confirm-digest `. +2. Run: `bash ~/.agents/skills/__SKILL_NAME__/scripts/remote.sh unlock --bundle --confirm-digest ` +3. The snapshot digest must be compared over a separate live channel. Never infer or auto-confirm it. The bundle is permanent secret key material; tell the user to transfer and handle it only through their own trusted channel, never by pasting it into agent chat. +4. Show the complete result, including the imported-envelope count and engine PID. +5. The advanced form with repeatable `--snapshot` plus `--identity` or `--identity-stdin` remains available when explicitly requested. + +If argument starts with "remote status": +1. Parse an optional `` and `--json`. +2. Run: `bash ~/.agents/skills/__SKILL_NAME__/scripts/remote.sh status [] [--json]` +3. Show the output to the user. + +If argument starts with "remote sync start": +1. Parse the required ``. +2. Run: `bash ~/.agents/skills/__SKILL_NAME__/scripts/remote.sh sync start ` +3. Show the output to the user. + +If argument starts with "remote disconnect": +1. Parse the required ``. +2. Run: `bash ~/.agents/skills/__SKILL_NAME__/scripts/remote.sh disconnect ` +3. Show the output to the user. + +If argument starts with "remote forget": +1. Parse the required ``. This permanently deletes that team's local roster, history, keys, trust, and sync state, but never changes the server. +2. Do not add `--yes` yourself. Run: `bash ~/.agents/skills/__SKILL_NAME__/scripts/remote.sh forget ` +3. The command requires the user to confirm in their terminal. If this agent has no interactive terminal, show the deletion summary and tell the user to rerun the displayed command directly; never bypass confirmation for them. + +If argument starts with "key generate" followed by an optional team name: +1. Run: `~/.agents/skills/__SKILL_NAME__/scripts/key.sh generate []` +2. Show the full output to the user, including the mandatory key-backup notice — do not summarize it away. + +If argument starts with "key show": +1. Parse an optional team name and `--reveal-secret`. +2. Run: `~/.agents/skills/__SKILL_NAME__/scripts/key.sh show [] [--reveal-secret]` +3. `--reveal-secret` requires a real interactive terminal and is refused in agent mode — if the user wants to reveal a secret, tell them to run it themselves directly in their own terminal rather than through you. +4. Show the output to the user. + +If argument starts with "key handoff" followed by a team name: +1. Parse optional `--out ` and run: `bash ~/.agents/skills/__SKILL_NAME__/scripts/key.sh handoff [--out ]` +2. The output bundle contains every epoch identity and is itself permanent secret key material. Never read it into agent chat or display its contents. +3. Show the bundle path, latest snapshot digest, and full secrecy warning. + +If argument starts with "key import" followed by a team name: +1. **Do not ask the user to paste the private identity into this chat, and do not run this command yourself.** This identity is a permanent secret. Tell the user to run this directly in their own terminal: + ``` + read -rsp 'Identity: ' IDENTITY; echo + printf '%s' "$IDENTITY" | ~/.agents/skills/__SKILL_NAME__/scripts/key.sh import --identity-stdin + unset IDENTITY + ``` +2. Ask them to paste back only the command's output (never the identity itself) once it's done. +3. **No advanced/automation env-var path is offered for key import** — not even a pre-existing, before-session variable. An identity file is a permanent secret; always use the human-in-own-terminal flow above. + +If argument starts with "key rotate" followed by a team name: +1. Rotation mints a replacement epoch for a team that already has a key and announces it on the roster journal. It requires an existing current key, an identity journal (connect or migrate the team first), and `age`; it refuses with a message naming whichever is missing. +2. Confirm with the user before running it. It changes the team's key state, and every other machine has to receive the new identity out of band. +3. Run: `bash ~/.agents/skills/__SKILL_NAME__/scripts/key.sh rotate ` +4. Show the output: epoch, key_id, and recipient fingerprint. The private key is never written to the journal. Revealing it needs `key show --key-id --reveal-secret`, which is refused in agent mode — tell the user to run that in their own terminal. +5. Messages before the acknowledged rotation boundary remain readable with the old key. + +Device pairing (`key request` / `key approve`) is not implemented — they are not `key.sh` subcommands, so a call prints usage and exits 1. If the user asks for one, tell them so instead of attempting to run it. diff --git a/scripts/drivers/types/devin/type.conf b/scripts/drivers/types/devin/type.conf new file mode 100644 index 000000000..998d7cad5 --- /dev/null +++ b/scripts/drivers/types/devin/type.conf @@ -0,0 +1,7 @@ +# agmsg agent-type manifest — read-only key=value DATA. NEVER sourced. +name=devin +template=template.md +cli=devin +detect_proc=devin devin-* +monitor=no +delivery_modes=off diff --git a/site/src/i18n/de.js b/site/src/i18n/de.js index 1e68d08e5..d5a52dd6d 100644 --- a/site/src/i18n/de.js +++ b/site/src/i18n/de.js @@ -70,7 +70,8 @@ export default { copilot: "GitHub Copilot in der Shell.", cursor: "Cursors Headless-CLI-Agent.", opencode: "Open-Source-Coding-Agent.", - "grok-build": "xAIs Build-/Coding-Agent.", + devin: "Autonomer KI-Software-Ingenieur-CLI.", + "grok-build": "Build/Coding-Agent von xAI.", hermes: "Leichtgewichtiger Relay-Agent.", antigravity: "Agentische Coding-Umgebung.", }, diff --git a/site/src/i18n/en.js b/site/src/i18n/en.js index e1364fe7f..a1e80a8f0 100644 --- a/site/src/i18n/en.js +++ b/site/src/i18n/en.js @@ -70,6 +70,7 @@ export default { copilot: "GitHub Copilot in the shell.", cursor: "Cursor's headless CLI agent.", opencode: "Open-source coding agent.", + devin: "Autonomous AI software engineer CLI.", "grok-build": "xAI's build/coding agent.", hermes: "Lightweight relay agent.", antigravity: "Agentic coding environment.", diff --git a/site/src/i18n/es.js b/site/src/i18n/es.js index 9d3979ffa..f48b6c930 100644 --- a/site/src/i18n/es.js +++ b/site/src/i18n/es.js @@ -69,8 +69,9 @@ export default { gemini: "El agente de codificación CLI de Google.", copilot: "GitHub Copilot en la terminal.", cursor: "El agente CLI de Cursor sin interfaz gráfica.", - opencode: "Agente de codificación de código abierto.", - "grok-build": "El agente de compilación/codificación de xAI.", + opencode: "Agente de código abierto.", + devin: "CLI de ingeniero de software de IA autónomo.", + "grok-build": "Agente de build/código de xAI.", hermes: "Agente de retransmisión ligero.", antigravity: "Entorno de codificación agéntico.", }, diff --git a/site/src/i18n/fr.js b/site/src/i18n/fr.js index 013b2fbe7..8827dcfe0 100644 --- a/site/src/i18n/fr.js +++ b/site/src/i18n/fr.js @@ -70,6 +70,7 @@ export default { copilot: "GitHub Copilot dans le shell.", cursor: "L'agent CLI headless de Cursor.", opencode: "Agent de codage open source.", + devin: "CLI d'ingénieur logiciel IA autonome.", "grok-build": "L'agent de build/codage de xAI.", hermes: "Agent relais léger.", antigravity: "Environnement de codage agentique.", diff --git a/site/src/i18n/ja.js b/site/src/i18n/ja.js index f90ca11bb..cc7385d3c 100644 --- a/site/src/i18n/ja.js +++ b/site/src/i18n/ja.js @@ -68,6 +68,7 @@ export default { copilot: "シェルで使うGitHub Copilot", cursor: "Cursorのヘッドレス版CLIエージェント", opencode: "オープンソースのコーディングエージェント", + devin: "自律型AIソフトウェアエンジニアCLI", "grok-build": "xAIのビルド/コーディングエージェント", hermes: "軽量なリレーエージェント", antigravity: "エージェント型のコーディング環境", diff --git a/site/src/i18n/ko.js b/site/src/i18n/ko.js index cd7e3dd1f..2165ee68d 100644 --- a/site/src/i18n/ko.js +++ b/site/src/i18n/ko.js @@ -68,6 +68,7 @@ export default { copilot: "셸에서 사용하는 GitHub Copilot.", cursor: "Cursor의 헤드리스 CLI 에이전트.", opencode: "오픈소스 코딩 에이전트.", + devin: "자율형 AI 소프트웨어 엔지니어 CLI.", "grok-build": "xAI의 빌드/코딩 에이전트.", hermes: "경량 릴레이 에이전트.", antigravity: "에이전틱 코딩 환경.", diff --git a/site/src/i18n/pt-BR.js b/site/src/i18n/pt-BR.js index 0086608f1..bb1186e1d 100644 --- a/site/src/i18n/pt-BR.js +++ b/site/src/i18n/pt-BR.js @@ -69,8 +69,9 @@ export default { gemini: "Agente de codificação de CLI do Google.", copilot: "GitHub Copilot no terminal.", cursor: "Agente de CLI headless do Cursor.", - opencode: "Agente de codificação open-source.", - "grok-build": "Agente de build/codificação da xAI.", + opencode: "Agente de codificação open source.", + devin: "CLI de engenheiro de software de IA autônomo.", + "grok-build": "Agente de build/código da xAI.", hermes: "Agente de relay leve.", antigravity: "Ambiente de codificação agêntico.", }, diff --git a/site/src/i18n/zh-CN.js b/site/src/i18n/zh-CN.js index 783e04f53..e7c8c125c 100644 --- a/site/src/i18n/zh-CN.js +++ b/site/src/i18n/zh-CN.js @@ -68,6 +68,7 @@ export default { copilot: "在终端中使用的 GitHub Copilot。", cursor: "Cursor 的无头 CLI 智能体。", opencode: "开源编程智能体。", + devin: "自主AI软件工程师CLI。", "grok-build": "xAI 的构建/编程智能体。", hermes: "轻量级中继智能体。", antigravity: "智能体编程环境。", diff --git a/site/src/i18n/zh-TW.js b/site/src/i18n/zh-TW.js index 550f753b6..12ae7e8e6 100644 --- a/site/src/i18n/zh-TW.js +++ b/site/src/i18n/zh-TW.js @@ -69,8 +69,9 @@ export default { gemini: "Google 的 CLI 編碼代理。", copilot: "在終端機中使用的 GitHub Copilot。", cursor: "Cursor 的無頭 CLI 代理。", - opencode: "開源編碼代理。", - "grok-build": "xAI 的建置／編碼代理。", + opencode: "開源程式設計代理。", + devin: "自主AI軟體工程師CLI。", + "grok-build": "xAI 的建置/程式設計代理。", hermes: "輕量級中繼代理。", antigravity: "代理式編碼環境。", }, diff --git a/site/src/layouts/Home.astro b/site/src/layouts/Home.astro index 2ae781075..c1dc160ca 100644 --- a/site/src/layouts/Home.astro +++ b/site/src/layouts/Home.astro @@ -33,6 +33,7 @@ const META = { copilot: { label: "Copilot CLI", logo: "/logos/copilot.svg", blurb: "GitHub Copilot in the shell." }, cursor: { label: "Cursor Agent", logo: null, blurb: "Cursor's headless CLI agent." }, opencode: { label: "OpenCode", logo: "/logos/opencode.svg", blurb: "Open-source coding agent.", mono: true }, + devin: { label: "Devin", logo: null, blurb: "Autonomous AI software engineer CLI." }, "grok-build": { label: "Grok Build", logo: "/logos/grok.svg", blurb: "xAI's build/coding agent.", mono: true }, hermes: { label: "Hermes", logo: "/logos/hermes.png", blurb: "Lightweight relay agent." }, antigravity: { label: "Antigravity", logo: "/logos/antigravity.png", blurb: "Agentic coding environment." }, diff --git a/tests/test_delivery.bats b/tests/test_delivery.bats index d99f562d3..e60866990 100644 --- a/tests/test_delivery.bats +++ b/tests/test_delivery.bats @@ -2658,6 +2658,38 @@ JSON wait 2>/dev/null || true } +# --- devin (manual-only: delivery_modes=off, no automatic hook) --- + +@test "delivery devin: status is manual/off" { + run bash "$SCRIPTS/delivery.sh" status devin "$TEST_PROJECT" + [ "$status" -eq 0 ] + [[ "$output" =~ "mode: off" ]] +} + +@test "delivery devin: rejects automatic modes" { + local mode + for mode in turn monitor both; do + run bash "$SCRIPTS/delivery.sh" set "$mode" devin "$TEST_PROJECT" + [ "$status" -ne 0 ] + [[ "$output" =~ "not supported for devin" ]] + done +} + +@test "delivery devin: rejects unknown mode" { + run bash "$SCRIPTS/delivery.sh" set bogus devin "$TEST_PROJECT" + [ "$status" -ne 0 ] + [[ "$output" =~ "Unknown mode" ]] +} + +@test "delivery devin: accepts off without error" { + run bash "$SCRIPTS/delivery.sh" set off devin "$TEST_PROJECT" + [ "$status" -eq 0 ] + [[ "$output" =~ "Delivery mode set to 'off'" ]] + [[ "$output" =~ "manual inbox checks only" ]] + [[ "$output" != *"AGMSG-DIRECTIVE"* ]] +} + + # --- grok-build (turn|off via a markdown rule file .grok/rules/agmsg.md) --- # Grok passive hooks can't inject (stdout is discarded), so grok delivers via the # rule-file self-poll model (like gemini/opencode): a .grok/rules/agmsg.md that diff --git a/tests/test_type_registry.bats b/tests/test_type_registry.bats index 07799d885..e6cb24304 100644 --- a/tests/test_type_registry.bats +++ b/tests/test_type_registry.bats @@ -29,11 +29,11 @@ write_node_launcher_fixtures() { printf '// stub node launcher fixture\n' > "$nd/nodetype-launcher.mjs" } -@test "type-registry: known_types lists the ten built-ins" { +@test "type-registry: known_types lists the eleven built-ins" { run env -i PATH="$PATH" bash -c \ "source '$SCRIPTS/lib/type-registry.sh'; agmsg_known_types | sort -u | paste -sd, -" [ "$status" -eq 0 ] - [ "$output" = "agmsg-app,antigravity,claude-code,codex,copilot,cursor,gemini,grok-build,hermes,opencode" ] + [ "$output" = "agmsg-app,antigravity,claude-code,codex,copilot,cursor,devin,gemini,grok-build,hermes,opencode" ] } @test "type-registry: is_known_type accepts a built-in and rejects a bogus type" { @@ -92,9 +92,9 @@ write_node_launcher_fixtures() { } @test "agent templates all explain that readable local history is not evidence a team is unencrypted (#682)" { - # scripts/drivers/types/*/template.md is nine independent copies with no + # scripts/drivers/types/*/template.md is ten independent copies with no # shared fragment (#676's exact shape) -- a loop with `[ -f ] || continue` - # alone would silently pass if the glob matched fewer than nine files (a + # alone would silently pass if the glob matched fewer than ten files (a # renamed/missing template), so the count is asserted explicitly rather # than just "every file found had it." local template count=0 @@ -105,9 +105,9 @@ write_node_launcher_fixtures() { || { echo "missing the e2ee-verification paragraph: $template" >&2; return 1; } done # agmsg-app has no template.md (spawnable=no -- it's the desktop app's own - # identity, not a CLI type), so nine is the whole set, not a lower bound a + # identity, not a CLI type), so ten is the whole set, not a lower bound a # silently-skipped file could still satisfy. - [ "$count" -eq 9 ] + [ "$count" -eq 10 ] } @test "the e2ee-verification explanation also appears in both remote-setup docs (#682)" { @@ -156,8 +156,8 @@ write_node_launcher_fixtures() { ! grep -qiE 'rotat(e|ion)[^.]*not available' "$surface" \ || { echo "still calls rotation unavailable: $surface" >&2; return 1; } done - # nine templates (agmsg-app has none) plus SKILL.md. - [ "$count" -eq 10 ] + # ten templates (agmsg-app has none) plus SKILL.md. + [ "$count" -eq 11 ] # Bind the claim to the code. If `rotate` ever stops being a subcommand the # surfaces above become wrong again, and this is the line that says so. @@ -170,8 +170,8 @@ write_node_launcher_fixtures() { grep -Fq 'To mint a replacement epoch instead:' "$BATS_TEST_DIRNAME/../scripts/key.sh" } -@test "type-registry: spawnable set is exactly eight of the ten built-ins (#277, #279)" { - # hermes deliberately stays out (#279): no known CLI mode starts it +@test "type-registry: spawnable set is exactly eight of the eleven built-ins (#277, #279)" { + # hermes and devin deliberately stay out (#279): no known CLI mode starts them # interactive with a seeded initial prompt. agmsg-app also stays out: it's # the desktop app itself (spawnable=no), not a spawnable agent type. run env -i PATH="$PATH" bash -c \ From d79c3544932458ac467b5ea1a1b92dc6fcb60821 Mon Sep 17 00:00:00 2001 From: m1kekad0 Date: Wed, 19 Aug 2026 20:37:55 +0900 Subject: [PATCH 02/34] docs: clarify Devin spawn support status AI-assisted-by: Gemini 3.7 Flash --- README.ja.md | 2 +- README.md | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/README.ja.md b/README.ja.md index 678ba90cf..0734fdb25 100644 --- a/README.ja.md +++ b/README.ja.md @@ -210,7 +210,7 @@ codex: --dangerously-skip-permissions: false # `false`の値はフラグ自体を出力しない ``` -10種類のエージェントタイプのうち8つがspawn可能 — `claude-code`、`codex`、`grok-build`、`cursor`、`gemini`、`antigravity`、`copilot`、`opencode`。`hermes` と `devin` は不可 — 初期プロンプトを事前に仕込んだインタラクティブセッションを開始するモードがない。macOSが主なターゲットで、LinuxとWindowsはベストエフォート（ターミナルが未対応の場合はissueまたはPRを歓迎）。ヘッドレス環境 — tmuxもなく使えるターミナルもない — はエージェントCLIがインタラクティブなターミナルを必要とするためエラーになる。 +10種類のエージェントタイプのうち8つがspawn可能 — `claude-code`、`codex`、`grok-build`、`cursor`、`gemini`、`antigravity`、`copilot`、`opencode`。`hermes` は、初期プロンプトを事前に仕込んだインタラクティブセッションを開始する既知のCLIモードがないためspawn不可（#279）。`devin` は、同等のインタラクティブ起動方法がまだ検証されていないため、現時点ではspawnableとして扱っていない。macOSが主なターゲットで、LinuxとWindowsはベストエフォート（ターミナルが未対応の場合はissueまたはPRを歓迎）。ヘッドレス環境 — tmuxもなく使えるターミナルもない — はエージェントCLIがインタラクティブなターミナルを必要とするためエラーになる。 ### spawnしたエージェントを終了する（`despawn`） diff --git a/README.md b/README.md index a6ccdfd3a..09551810a 100644 --- a/README.md +++ b/README.md @@ -211,7 +211,7 @@ codex: --dangerously-skip-permissions: false # a `false` value suppresses the flag entirely ``` -Eight of the ten agent types are spawnable — `claude-code`, `codex`, `grok-build`, `cursor`, `gemini`, `antigravity`, `copilot`, `opencode`. `hermes` and `devin` are not: their CLIs do not have a known interactive session boot mode pre-seeded with an initial prompt. macOS is the primary target; Linux and Windows are best-effort (please open an issue/PR if your terminal isn't handled). Headless environments — no tmux **and** no usable terminal — error out, since the agent CLIs need an interactive terminal. +Eight of the ten agent types are spawnable — `claude-code`, `codex`, `grok-build`, `cursor`, `gemini`, `antigravity`, `copilot`, `opencode`. `hermes` is not spawnable because its CLI has no known mode that starts an interactive session pre-seeded with an initial prompt (#279). `devin` is currently not marked spawnable because an equivalent interactive boot mode has not yet been verified. macOS is the primary target; Linux and Windows are best-effort (please open an issue/PR if your terminal isn't handled). Headless environments — no tmux **and** no usable terminal — error out, since the agent CLIs need an interactive terminal. ### Tear down a spawned agent (`despawn`) From 39c8e573b7946e25bdf7e7aaf3fcb4d6f7f4275d Mon Sep 17 00:00:00 2001 From: m1kekad0 Date: Wed, 19 Aug 2026 20:53:16 +0900 Subject: [PATCH 03/34] chore: narrow Devin agent type diff AI-assisted-by: Gemini 3.7 Flash --- docs/agent-types.md | 2 +- site/src/i18n/de.js | 4 ++-- site/src/i18n/en.js | 2 +- site/src/i18n/es.js | 6 +++--- site/src/i18n/fr.js | 2 +- site/src/i18n/ja.js | 2 +- site/src/i18n/ko.js | 2 +- site/src/i18n/pt-BR.js | 6 +++--- site/src/i18n/zh-CN.js | 2 +- site/src/i18n/zh-TW.js | 6 +++--- tests/test_delivery.bats | 1 - 11 files changed, 17 insertions(+), 18 deletions(-) diff --git a/docs/agent-types.md b/docs/agent-types.md index ef1a17a61..8a895ff3f 100644 --- a/docs/agent-types.md +++ b/docs/agent-types.md @@ -1,7 +1,7 @@ # Agent types agmsg supports several agent runtimes — claude-code, codex, gemini, antigravity, -copilot, opencode, hermes, cursor, devin, grok-build — and each is described by a small **manifest** so that the rest +copilot, opencode, hermes, cursor, devin — and each is described by a small **manifest** so that the rest of agmsg (detection, the join whitelist, spawn, and delivery routing) discovers it from data instead of hardcoded `case` arms. diff --git a/site/src/i18n/de.js b/site/src/i18n/de.js index d5a52dd6d..bdd93d71c 100644 --- a/site/src/i18n/de.js +++ b/site/src/i18n/de.js @@ -70,10 +70,10 @@ export default { copilot: "GitHub Copilot in der Shell.", cursor: "Cursors Headless-CLI-Agent.", opencode: "Open-Source-Coding-Agent.", - devin: "Autonomer KI-Software-Ingenieur-CLI.", - "grok-build": "Build/Coding-Agent von xAI.", + "grok-build": "xAIs Build-/Coding-Agent.", hermes: "Leichtgewichtiger Relay-Agent.", antigravity: "Agentische Coding-Umgebung.", + devin: "Autonomer KI-Software-Ingenieur-CLI.", }, }, showcase: { diff --git a/site/src/i18n/en.js b/site/src/i18n/en.js index a1e80a8f0..69fd15a35 100644 --- a/site/src/i18n/en.js +++ b/site/src/i18n/en.js @@ -70,10 +70,10 @@ export default { copilot: "GitHub Copilot in the shell.", cursor: "Cursor's headless CLI agent.", opencode: "Open-source coding agent.", - devin: "Autonomous AI software engineer CLI.", "grok-build": "xAI's build/coding agent.", hermes: "Lightweight relay agent.", antigravity: "Agentic coding environment.", + devin: "Autonomous AI software engineer CLI.", }, }, showcase: { diff --git a/site/src/i18n/es.js b/site/src/i18n/es.js index f48b6c930..17ba3914d 100644 --- a/site/src/i18n/es.js +++ b/site/src/i18n/es.js @@ -69,11 +69,11 @@ export default { gemini: "El agente de codificación CLI de Google.", copilot: "GitHub Copilot en la terminal.", cursor: "El agente CLI de Cursor sin interfaz gráfica.", - opencode: "Agente de código abierto.", - devin: "CLI de ingeniero de software de IA autónomo.", - "grok-build": "Agente de build/código de xAI.", + opencode: "Agente de codificación de código abierto.", + "grok-build": "El agente de compilación/codificación de xAI.", hermes: "Agente de retransmisión ligero.", antigravity: "Entorno de codificación agéntico.", + devin: "CLI de ingeniero de software de IA autónomo.", }, }, showcase: { diff --git a/site/src/i18n/fr.js b/site/src/i18n/fr.js index 8827dcfe0..577e80cbd 100644 --- a/site/src/i18n/fr.js +++ b/site/src/i18n/fr.js @@ -70,10 +70,10 @@ export default { copilot: "GitHub Copilot dans le shell.", cursor: "L'agent CLI headless de Cursor.", opencode: "Agent de codage open source.", - devin: "CLI d'ingénieur logiciel IA autonome.", "grok-build": "L'agent de build/codage de xAI.", hermes: "Agent relais léger.", antigravity: "Environnement de codage agentique.", + devin: "CLI d'ingénieur logiciel IA autonome.", }, }, showcase: { diff --git a/site/src/i18n/ja.js b/site/src/i18n/ja.js index cc7385d3c..a7757fa3b 100644 --- a/site/src/i18n/ja.js +++ b/site/src/i18n/ja.js @@ -68,10 +68,10 @@ export default { copilot: "シェルで使うGitHub Copilot", cursor: "Cursorのヘッドレス版CLIエージェント", opencode: "オープンソースのコーディングエージェント", - devin: "自律型AIソフトウェアエンジニアCLI", "grok-build": "xAIのビルド/コーディングエージェント", hermes: "軽量なリレーエージェント", antigravity: "エージェント型のコーディング環境", + devin: "自律型AIソフトウェアエンジニアCLI", }, }, showcase: { diff --git a/site/src/i18n/ko.js b/site/src/i18n/ko.js index 2165ee68d..0bafb5ea2 100644 --- a/site/src/i18n/ko.js +++ b/site/src/i18n/ko.js @@ -68,10 +68,10 @@ export default { copilot: "셸에서 사용하는 GitHub Copilot.", cursor: "Cursor의 헤드리스 CLI 에이전트.", opencode: "오픈소스 코딩 에이전트.", - devin: "자율형 AI 소프트웨어 엔지니어 CLI.", "grok-build": "xAI의 빌드/코딩 에이전트.", hermes: "경량 릴레이 에이전트.", antigravity: "에이전틱 코딩 환경.", + devin: "자율형 AI 소프트웨어 엔지니어 CLI.", }, }, showcase: { diff --git a/site/src/i18n/pt-BR.js b/site/src/i18n/pt-BR.js index bb1186e1d..17dd4ee26 100644 --- a/site/src/i18n/pt-BR.js +++ b/site/src/i18n/pt-BR.js @@ -69,11 +69,11 @@ export default { gemini: "Agente de codificação de CLI do Google.", copilot: "GitHub Copilot no terminal.", cursor: "Agente de CLI headless do Cursor.", - opencode: "Agente de codificação open source.", - devin: "CLI de engenheiro de software de IA autônomo.", - "grok-build": "Agente de build/código da xAI.", + opencode: "Agente de codificação open-source.", + "grok-build": "Agente de build/codificação da xAI.", hermes: "Agente de relay leve.", antigravity: "Ambiente de codificação agêntico.", + devin: "CLI de engenheiro de software de IA autônomo.", }, }, showcase: { diff --git a/site/src/i18n/zh-CN.js b/site/src/i18n/zh-CN.js index e7c8c125c..cf3a35c1a 100644 --- a/site/src/i18n/zh-CN.js +++ b/site/src/i18n/zh-CN.js @@ -68,10 +68,10 @@ export default { copilot: "在终端中使用的 GitHub Copilot。", cursor: "Cursor 的无头 CLI 智能体。", opencode: "开源编程智能体。", - devin: "自主AI软件工程师CLI。", "grok-build": "xAI 的构建/编程智能体。", hermes: "轻量级中继智能体。", antigravity: "智能体编程环境。", + devin: "自主AI软件工程师CLI。", }, }, showcase: { diff --git a/site/src/i18n/zh-TW.js b/site/src/i18n/zh-TW.js index 12ae7e8e6..b5d62acad 100644 --- a/site/src/i18n/zh-TW.js +++ b/site/src/i18n/zh-TW.js @@ -69,11 +69,11 @@ export default { gemini: "Google 的 CLI 編碼代理。", copilot: "在終端機中使用的 GitHub Copilot。", cursor: "Cursor 的無頭 CLI 代理。", - opencode: "開源程式設計代理。", - devin: "自主AI軟體工程師CLI。", - "grok-build": "xAI 的建置/程式設計代理。", + opencode: "開源編碼代理。", + "grok-build": "xAI 的建置／編碼代理。", hermes: "輕量級中繼代理。", antigravity: "代理式編碼環境。", + devin: "自主AI軟體工程師CLI。", }, }, showcase: { diff --git a/tests/test_delivery.bats b/tests/test_delivery.bats index e60866990..a4ef91ea0 100644 --- a/tests/test_delivery.bats +++ b/tests/test_delivery.bats @@ -2689,7 +2689,6 @@ JSON [[ "$output" != *"AGMSG-DIRECTIVE"* ]] } - # --- grok-build (turn|off via a markdown rule file .grok/rules/agmsg.md) --- # Grok passive hooks can't inject (stdout is discarded), so grok delivers via the # rule-file self-poll model (like gemini/opencode): a .grok/rules/agmsg.md that From 0dc0e4ccb8c63b905689134027c6c0ea6451f304 Mon Sep 17 00:00:00 2001 From: m1kekad0 Date: Wed, 19 Aug 2026 23:14:09 +0900 Subject: [PATCH 04/34] test: make Devin assertions enforceable AI-assisted-by: Gemini 3.7 Flash --- tests/test_delivery.bats | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/test_delivery.bats b/tests/test_delivery.bats index a4ef91ea0..6bfffe4a7 100644 --- a/tests/test_delivery.bats +++ b/tests/test_delivery.bats @@ -2684,9 +2684,9 @@ JSON @test "delivery devin: accepts off without error" { run bash "$SCRIPTS/delivery.sh" set off devin "$TEST_PROJECT" [ "$status" -eq 0 ] - [[ "$output" =~ "Delivery mode set to 'off'" ]] - [[ "$output" =~ "manual inbox checks only" ]] - [[ "$output" != *"AGMSG-DIRECTIVE"* ]] + grep -qF "Delivery mode set to 'off'" <<<"$output" + grep -qF "manual inbox checks only" <<<"$output" + refute grep -qF "AGMSG-DIRECTIVE" <<<"$output" } # --- grok-build (turn|off via a markdown rule file .grok/rules/agmsg.md) --- From a336005b2326ab0a964e6a8248ce9ed836cd362a Mon Sep 17 00:00:00 2001 From: fujibee Date: Thu, 10 Sep 2026 08:20:22 -0700 Subject: [PATCH 05/34] test(teardown): bound the holder wait, and stop the probe from counting itself (#1123) test_teardown_reap.bats hung the CI shard it landed in: 419 of 435 tests completed, then thirty minutes of silence and a terminated job leaving xargs and several bash orphans. The cause was not the reaper. _start_holder ran inside a command substitution and started a holder that never closed stdout and stderr, so the capturing pipe never reached EOF and _start_holder itself did not return. Close the holder's streams, and bound the readiness wait so reaching the bound is non-zero rather than silent. Bounding it then exposed a second defect, found in review: the process search passed the directory to look for through awk's own argv, so the ps snapshot matched the probe itself and the reaper always reached its ceiling. Pass it in the environment instead, and assert directly that a scan with nothing holding the directory reports nothing. Measured before and after, isolating the stall: before, no output for sixty seconds and killed; after, no timeout, and the two no-holder controls finish in under a second against the six-second ceiling. Tests only; no shipped file changes. --- tests/test_helper.bash | 93 ++++++++++++++++++++++- tests/test_teardown_reap.bats | 134 ++++++++++++++++++++++++++++++++++ 2 files changed, 226 insertions(+), 1 deletion(-) create mode 100644 tests/test_teardown_reap.bats diff --git a/tests/test_helper.bash b/tests/test_helper.bash index 1e80586dd..a9c481ab6 100644 --- a/tests/test_helper.bash +++ b/tests/test_helper.bash @@ -34,8 +34,99 @@ setup_test_env() { mkdir -p "$HOME" } +# PIDs (one per line, this shell excluded) whose command line references . +# The detached codex children — codex-bridge-launcher.sh and the codex-bridge.js it +# starts (codex-monitor.sh spawns the launcher with `… &`, "outlives this script") — +# resolve their SKILL_DIR from their own script path, so their argv carries +# TEST_SKILL_DIR. The launcher records no pidfile of its own, so a pidfile sweep cannot +# reach it; the command line is what names it. Unix uses ps; on Git Bash ps enumerates +# MSYS processes, which the launcher/bridge are, so it reaches them there too. +# +# LIMIT (named deliberately, not a defect): this matches only processes that carry +# $dir IN THEIR ARGV. A process whose CWD is inside $dir but whose argv does not name +# it would NOT be found. The two known holders are argv-visible today — the launcher +# resolves SKILL_DIR from its own script path (argv[0]), and the bridge receives +# --workspace-root — so they are caught; but that is a property of THOSE two, not +# a guarantee about any future holder. A cwd/open-fd sweep (lsof) would close the gap; +# it is deliberately NOT used because lsof is slow and this runs in EVERY test's +# teardown — too heavy for the ~all tests that hold nothing. If a future detached child +# holds $dir without naming it in argv, revisit (add an lsof pass gated on the rm +# actually failing, so the cost is paid only when it is needed). +_pids_referencing_dir() { # + ps -eo pid=,args= 2>/dev/null | + AGMSG_REAP_DIR="$1" awk -v me="$$" 'index($0, ENVIRON["AGMSG_REAP_DIR"]) { if ($1+0 != me+0) print $1 }' +} + +# Reap any process still holding $TEST_SKILL_DIR, then let handles release, BEFORE the +# rm. Those detached children keep writing $TEST_SKILL_DIR/run after the test body +# returns and are in no pidset the tests kill, so the bare rm below races them and fails +# `rm: Directory not empty` (or, on Windows, `Device or resource busy` on the bridge's +# open messages.db). #662 == #1036 == #1049. +# +# Scope is $TEST_SKILL_DIR ITSELF — a unique mktemp path — so matching it in process +# args cannot reach a developer's live bridge or another test's processes; this is never +# a blanket `pkill codex-bridge.js`. Guarded to a temp path so a mis-set variable can +# never turn the scan loose on a short/rooty prefix. A single `ps` for the ~all tests +# that spawn nothing. +# +# The SIGTERM→wait→SIGKILL sequence is EXERCISED by tests/test_teardown_reap.bats (kill, +# scope-safety, no-op, guard); whether the wait budget is long enough on a load-3-digit +# host, and whether killing a holder RELEASES the Windows file handle before the rm, are +# both timing/OS facts this repo cannot measure on the author's loaded machine — CI +# (dedicated runners, Windows leg) measures them. Written as designed-and-static-checked, +# NOT as "measured", per the day's rule that a claim states how it was verified (#1036). +_reap_test_skill_dir_procs() { + local dir="${TEST_SKILL_DIR:-}" + case "$dir" in + ""|/|/tmp|/var|/private|/usr|"$HOME") return 0 ;; + esac + case "$dir" in + /tmp/*|/private/*|/var/folders/*|/private/var/folders/*) : ;; + *) + # Outside the well-known temp roots, allow ONLY under a TMPDIR that is set AND a + # real path — never unset, "", or "/". Resolve and VALIDATE the prefix before using + # it as a pattern: a pattern assembled from an empty prefix ("${TMPDIR:+…}" with + # TMPDIR unset, or "${TMPDIR%/}" with TMPDIR="/") degenerates to match ANY non-empty + # dir. This guards a KILL, so the loose failure kills EXTRA processes, not nothing + # (co2 BLOCKING). Strip the trailing slash first, then require the result non-empty, + # so unset / "" / "/" all fail closed. Only then is "$_tmp" safe as a pattern prefix. + local _tmp="${TMPDIR:-}"; _tmp="${_tmp%/}" + [ -n "$_tmp" ] || return 0 + case "$dir" in "$_tmp"/?*) : ;; *) return 0 ;; esac + ;; + esac + local pids tries=0 sig p + while :; do + pids="$(_pids_referencing_dir "$dir")" + [ -n "$pids" ] || return 0 + # Escalate to SIGKILL quickly (after ~0.3s of SIGTERM): a detached launcher may not + # act on SIGTERM, and this is a teardown, not a graceful shutdown. SIGKILL is + # uncatchable, so once sent the process WILL die — the only remaining wait is for ps + # to stop listing it, which a heavily loaded host can slow. So keep re-checking up + # to ~6s (a bound only ever reached when something is genuinely stuck; the ~all tests + # that hold nothing return on the first check above), then return and let the rm + # surface anything still there. The 6s headroom is what covers a load-3-digit host. + sig=TERM; [ "$tries" -ge 3 ] && sig=KILL + for p in $pids; do kill "-$sig" "$p" 2>/dev/null || true; done + [ "$tries" -ge 60 ] && return 1 + sleep 0.1 2>/dev/null || true + tries=$((tries + 1)) + done +} + teardown_test_env() { - rm -rf "$TEST_SKILL_DIR" + # Try the plain rm FIRST, and only reap when it actually fails. The reaper's scan is a + # full `ps -eo pid=,args=`; running it in EVERY teardown would add that cost to all of + # the (vast majority of) tests that hold nothing — across the suite's hundreds of tests + # that dominates the runtime and pushes CI shards over their timeout. The race it fixes + # is rare (only the codex tests spawn the detached launcher), and it announces itself + # as a non-zero rm ("Directory not empty" / "Device or resource busy"), so pay the cost + # exactly there: on failure, reap the TEST_SKILL_DIR-scoped holders and retry. + rm -rf "$TEST_SKILL_DIR" 2>/dev/null && return 0 + local reap_status=0 rm_status=0 + _reap_test_skill_dir_procs || reap_status=$? + rm -rf "$TEST_SKILL_DIR" || rm_status=$? + [ "$reap_status" -eq 0 ] && [ "$rm_status" -eq 0 ] } # Skip a test on native Windows / Git Bash (MSYS/MINGW/Cygwin). Use ONLY for diff --git a/tests/test_teardown_reap.bats b/tests/test_teardown_reap.bats new file mode 100644 index 000000000..2025ece13 --- /dev/null +++ b/tests/test_teardown_reap.bats @@ -0,0 +1,134 @@ +#!/usr/bin/env bats +# +# teardown_test_env must kill a detached process still holding $TEST_SKILL_DIR before +# it removes the tree. The detached codex children (codex-bridge-launcher.sh and the +# codex-bridge.js it starts) outlive the test body and keep writing $TEST_SKILL_DIR/run, +# so the bare rm races them and fails "Directory not empty" (#662 == #1036 == #1049). +# The reaper is scoped to the unique mktemp $TEST_SKILL_DIR so it can never reach a real +# bridge or another test — that scope safety is asserted here, not just the kill. +# +# Assertions use plain commands / `refute`, never a non-last `[[ ]]` or `! cmd` (#670). + +load test_helper + +setup() { setup_test_env; } +teardown() { teardown_test_env; } + +# Spawn a detached holder whose argv carries $TEST_SKILL_DIR (as the launcher's does), +# keeping run/ busy. Echoes its pid once it is actually writing run/. +_start_holder() { + mkdir -p "$TEST_SKILL_DIR/run" + local holder="$TEST_SKILL_DIR/scripts/holder.sh" + cat > "$holder" <<'H' +#!/usr/bin/env bash +d="$1"; while :; do : > "$d/run/held.$$"; sleep 0.05; done +H + chmod +x "$holder" + # ORPHAN it in a subshell (the subshell exits, the holder reparents to init) so it + # matches the real detached launcher — and so that after the reaper's SIGKILL it is + # reaped by init rather than lingering as a zombie of this test shell, which would + # still answer `kill -0` and defeat the "it is dead" assertion. It records its own pid. + # Close the command substitution's stdout/stderr: leaving either descriptor open in + # the orphan makes the caller wait for EOF forever before it can reap the holder. + ( "$holder" "$TEST_SKILL_DIR" /dev/null 2>&1 & printf '%s\n' "$!" > "$TEST_SKILL_DIR/holder.pid" ) + local hp; hp="$(cat "$TEST_SKILL_DIR/holder.pid" 2>/dev/null)" + _wait_for_holder_ready "$TEST_SKILL_DIR/run/held.$hp" || { + echo "holder $hp did not become ready before the bounded wait expired" >&2 + return 1 + } + printf '%s\n' "$hp" +} + +# Wait for the holder to prove it is writing run/, with an explicit ceiling. Keep the +# knobs injectable so the timeout behavior can be tested without adding five seconds +# to the suite. +_wait_for_holder_ready() { + local marker="$1" ticks="${2:-100}" interval="${3:-0.05}" n=0 + while [ ! -e "$marker" ] && [ "$n" -lt "$ticks" ]; do + sleep "$interval" + n=$((n + 1)) + done + [ -e "$marker" ] +} + +@test "holder readiness times out and fails instead of waiting forever" { + run _wait_for_holder_ready "$TEST_SKILL_DIR/never-ready" 2 0.01 + [ "$status" -ne 0 ] +} + +@test "teardown reaper kills a detached process holding TEST_SKILL_DIR (#662)" { + local hp; hp="$(_start_holder)" + kill -0 "$hp" # holder is alive and holding run/ + _reap_test_skill_dir_procs # THE FIX — a no-op reaper leaves it alive (mutation) + refute kill -0 "$hp" # reaper killed it, so run/ is free for the rm +} + +@test "teardown reaper leaves a process that does NOT reference TEST_SKILL_DIR alone (scope safety)" { + # The reaper must never reach a developer's live bridge or another test's process. + # A plain `sleep` whose argv does not contain this test's mktemp dir must survive. + sleep 30 & + local other=$! + _reap_test_skill_dir_procs + kill -0 "$other" # untouched + kill "$other" 2>/dev/null || true +} + +@test "teardown reaper is a no-op when nothing holds TEST_SKILL_DIR" { + run _reap_test_skill_dir_procs + [ "$status" -eq 0 ] +} + +@test "teardown process scan does not report its own awk probe" { + run _pids_referencing_dir "$TEST_SKILL_DIR" + [ "$status" -eq 0 ] + [ -z "$output" ] +} + +@test "teardown reaper fails when its bounded wait expires" { + # Keep the process list non-empty while replacing the side effects with no-ops. The + # 60-tick ceiling then runs quickly and this test proves timeout is not reported green. + _pids_referencing_dir() { printf '%s\n' 999999; } + kill() { :; } + sleep() { :; } + run _reap_test_skill_dir_procs + [ "$status" -ne 0 ] +} + +@test "teardown reaper refuses to scan when TEST_SKILL_DIR is not a temp path (guard)" { + # A mis-set TEST_SKILL_DIR must never turn the scan loose on a short/rooty prefix + # (which would match — and kill — nearly every process). + sleep 30 & + local other=$! + TEST_SKILL_DIR="/" _reap_test_skill_dir_procs + TEST_SKILL_DIR="/usr" _reap_test_skill_dir_procs + kill -0 "$other" # nothing was scanned or killed for a rooty dir + kill "$other" 2>/dev/null || true +} + +@test "teardown reaper fails closed on a NON-temp TEST_SKILL_DIR for a degenerate TMPDIR: unset / empty / root (#662, co2 BLOCKING)" { + # utildev measured that ubuntu-latest's `mktemp -d` uses /tmp with TMPDIR UNSET, so a + # guard pattern assembled from $TMPDIR degenerates to ?* there and would turn this KILL + # loose on CI Linux. Prove fail-closed: with a NON-temp TEST_SKILL_DIR and TMPDIR unset, + # "", or "/", a probe whose argv carries that dir must SURVIVE. A bare `sleep` cannot + # show this — the degenerate scan would find nothing to kill and pass vacuously — so the + # probe gives the scan a real target, and its survival is what distinguishes refuse from + # scan-and-kill. (Mutation: restore the old `"${TMPDIR:+${TMPDIR%/}/}"?*` guard and this + # reds — the probe gets killed under the non-temp dir.) + local marker="/agmsg-nontemp-probe-$$" + local probe="$TEST_SKILL_DIR/scripts/probe.sh" + mkdir -p "$(dirname "$probe")" + printf '%s\n' '#!/usr/bin/env bash' 'while :; do sleep 0.1; done' > "$probe" + chmod +x "$probe" + local pp + _spawn_probe() { + # Orphaned (subshell) so it reparents to init like the real detached launcher. Its + # argv carries $marker (so the reaper scoped to $marker would hit it if it degenerated) + # AND $TEST_SKILL_DIR (the probe.sh path), so the real teardown reaps it afterwards. + ( "$probe" "$marker" & printf '%s\n' "$!" > "$TEST_SKILL_DIR/probe.pid" ) + pp="$(cat "$TEST_SKILL_DIR/probe.pid")" + kill -0 "$pp" # running, so "survives" below is meaningful rather than a race + } + _spawn_probe; ( unset TMPDIR; TEST_SKILL_DIR="$marker" _reap_test_skill_dir_procs ); kill -0 "$pp"; kill "$pp" 2>/dev/null || true + _spawn_probe; TMPDIR="" TEST_SKILL_DIR="$marker" _reap_test_skill_dir_procs; kill -0 "$pp"; kill "$pp" 2>/dev/null || true + _spawn_probe; TMPDIR="/" TEST_SKILL_DIR="$marker" _reap_test_skill_dir_procs; kill -0 "$pp"; kill "$pp" 2>/dev/null || true +} From 59dc385cbd45733b0e41619a97fa5889bf5a4ea8 Mon Sep 17 00:00:00 2001 From: fujibee Date: Fri, 11 Sep 2026 18:12:35 -0700 Subject: [PATCH 06/34] =?UTF-8?q?ci(tests):=20shard=20the=20bats=20suite?= =?UTF-8?q?=205=20ways,=20not=204=20=E2=80=94=20the=20main=20twin=20of=20t?= =?UTF-8?q?he=20#1107=20fix=20(#1162)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The same change already landed on the terminal-driver integration branch; this is the main-targeted twin so main PRs stop hitting the macOS 30-min cap that the 4-shard split reaches on this suite too. shard-tests.sh seeds each pinned file at one shard's average share (total_tests/total), which grows with the suite while the pinned files do not, over-reserving the two pin shards and pushing the real load onto the non-pinned ones. main is 1742 @tests, so the worst-case non-pinned shard total*(N+2)/N^2 drops from 653 (N=4) to 488 (N=5), back under the cap with margin. Five, not six, because the account runs at most five macOS jobs at once (the concurrency header). test_ci_workflow.bats reads SHARD_TOTAL and the matrix and follows the /N name; it is green at 5. This does not fix the scaling property (pin_seed still grows with the suite); that re-fit is #847. --- .github/workflows/tests.yml | 20 +++++++++++++++++--- tests/test_ci_workflow.bats | 4 ++-- 2 files changed, 19 insertions(+), 5 deletions(-) diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index 544c5896c..3aee94ebb 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -104,7 +104,21 @@ concurrency: env: # Number of parallel bats shards per OS. The matrix and the shard helper must # stay in lockstep; the stable summary job below verifies that they do. - SHARD_TOTAL: 4 + # + # 4 -> 5 (#1107). Same change already on the terminal-driver integration branch; + # this is the main-targeted twin so main PRs stop hitting the same macOS cap. + # shard-tests.sh seeds each pinned file at one shard's AVERAGE share + # (total_tests/total), which grows with the suite while the pinned files do not, + # over-reserving the two pin shards and pushing the real load onto the non-pin + # ones. main is 1742 @tests here, so the worst-case non-pin shard + # (total*(N+2)/N^2) is 653 at N=4 and 488 at N=5 -- and a 4-shard macOS leg was + # already reaching the 30-min cap while still passing tests (a slow shard, not a + # hang: the workflow's own #848/#857 note). Five, not six, because the account + # runs at most FIVE macOS jobs at once (the concurrency header): a sixth would + # queue behind the other five of its own run. This does NOT fix the scaling + # property (pin_seed still grows, count still under-values wait-dominant files); + # that re-fit is #847. + SHARD_TOTAL: 5 # The Windows legs' sqlite3, pinned three ways (#824). # # This comment used to end "one trip to chocolatey, which is the only time @@ -249,7 +263,7 @@ jobs: # the required contexts are `bats` (the aggregate) and the Windows install # leg, and neither is renamed, so branch protection sees exactly what it # saw. `needs` is one of the contexts a job name may read. - name: bats (${{ matrix.os }} ${{ matrix.shard }}/4)${{ needs.changes.outputs.docs_only == 'true' && ' — docs-only, suite skipped' || '' }} + name: bats (${{ matrix.os }} ${{ matrix.shard }}/5)${{ needs.changes.outputs.docs_only == 'true' && ' — docs-only, suite skipped' || '' }} needs: changes # Run even if `changes` somehow failed/was skipped — fail open to the full # suite rather than leaving this REQUIRED check unreported (which would @@ -283,7 +297,7 @@ jobs: fail-fast: false matrix: os: [ubuntu-latest, macos-latest] - shard: [1, 2, 3, 4] + shard: [1, 2, 3, 4, 5] steps: - uses: actions/checkout@v4 diff --git a/tests/test_ci_workflow.bats b/tests/test_ci_workflow.bats index 87fa6fc38..88edbc1a3 100644 --- a/tests/test_ci_workflow.bats +++ b/tests/test_ci_workflow.bats @@ -50,7 +50,7 @@ local workflow="$BATS_TEST_DIRNAME/../.github/workflows/tests.yml" # 1. The shard job's name carries the marker, keyed on the docs_only output. # Shard checks are not required contexts, so the name may vary. - grep -Fq "name: bats (\${{ matrix.os }} \${{ matrix.shard }}/4)\${{ needs.changes.outputs.docs_only == 'true' && ' — docs-only, suite skipped' || '' }}" "$workflow" + grep -Fq "name: bats (\${{ matrix.os }} \${{ matrix.shard }}/5)\${{ needs.changes.outputs.docs_only == 'true' && ' — docs-only, suite skipped' || '' }}" "$workflow" # 2. The aggregate -- the required context -- keeps its exact name, once, # unconditionally. [ "$(grep -c '^ name: bats$' "$workflow")" -eq 1 ] @@ -72,7 +72,7 @@ # The mutation took: the marker is gone from the copy. if grep -Fq "docs-only, suite skipped' || ''" "$mutant"; then false; fi # ...and the name pin no longer matches it. - if grep -Fq "name: bats (\${{ matrix.os }} \${{ matrix.shard }}/4)\${{ needs.changes.outputs.docs_only == 'true' && ' — docs-only, suite skipped' || '' }}" "$mutant"; then false; fi + if grep -Fq "name: bats (\${{ matrix.os }} \${{ matrix.shard }}/5)\${{ needs.changes.outputs.docs_only == 'true' && ' — docs-only, suite skipped' || '' }}" "$mutant"; then false; fi } # The suite has to run on the shape that actually gets dogfooded. A PR is only From 9cec2d4272b3f6ee26cde3b71e83c32c80edf3dd Mon Sep 17 00:00:00 2001 From: masa6161 <14049926+masa6161@users.noreply.github.com> Date: Sat, 12 Sep 2026 10:34:46 +0900 Subject: [PATCH 07/34] perf(codex): batch the rollout mtime scan (#1035) (#1037) agmsg_newest_rollout_files() forked a command-substitution subshell, uname, and stat for every rollout file; on Windows/MSYS2, where process creation is far costlier than a Linux fork, an accumulated rollout history turned the codex SessionStart hook into minutes of synchronous blocking (measured: 2675 files, ~12 min). Spawn paths always reach this scan because CODEX_THREAD_ID is not exported there. Replace the per-file loop with one find -print0 | compat_files_mtime_0 pipeline; the new compat helper reads NUL-separated paths and emits \t through a single xargs -0 stat per argv batch, sorted once. Measured on Windows: 200 files 35.5 s -> 0.15 s; 2675 files ~12 min -> 1.33 s. A file that disappears between listing and stat is now omitted rather than emitted as an mtime=0 entry. --- scripts/drivers/types/codex/_session-start.sh | 33 +++++++++++-------- scripts/lib/compat.sh | 29 ++++++++++++++++ 2 files changed, 49 insertions(+), 13 deletions(-) diff --git a/scripts/drivers/types/codex/_session-start.sh b/scripts/drivers/types/codex/_session-start.sh index 1aa55fb9b..9375ba38f 100644 --- a/scripts/drivers/types/codex/_session-start.sh +++ b/scripts/drivers/types/codex/_session-start.sh @@ -25,7 +25,17 @@ # portable compat_file_mtime, since `find -printf` is GNU-only (no # `-printf` on macOS/BSD find, which this repo also has to support). See #416. agmsg_newest_rollout_files() { - local dir="$1" limit="$2" f mtime + local dir="$1" limit="$2" + # Batch the mtime lookup (compat_files_mtime_0) instead of running + # `mtime=$(compat_file_mtime "$f")` in a per-file loop: that substitution + # forks a subshell + uname + stat for EVERY rollout, and on Windows/MSYS2 + # -- where process creation is orders of magnitude costlier than a Linux + # fork -- the scan measured ~12 minutes against 2582 accumulated rollouts. + # This function runs inside the synchronous SessionStart hook, and + # agmsg_resolve_codex_thread calls it up to 3 times, so the per-file form + # blocks Codex startup for tens of minutes once a machine has real rollout + # history. One stat per argv batch resolves the same list in seconds. + # # `head -n "$limit"` here would close its read end after $limit lines while # `sort` may still be writing -- under this caller's `set -euo pipefail`, # that SIGPIPEs `sort` (status 141) and pipefail surfaces it as the whole @@ -36,18 +46,15 @@ agmsg_newest_rollout_files() { # different mechanism. awk reads its input through to EOF regardless of # `n` (only *printing* stops early), so `sort` is always fully drained and # never SIGPIPEd. - # `|| true` on the mtime lookup: under set -e, a plain `var=$(cmd)` - # assignment DOES abort on cmd's failure (unlike a substitution used inside - # a test/conditional). A rollout that `find` listed but that Codex deletes - # or rotates before `stat` runs on it (a real possibility across the ~1-2s - # this loop can take with hundreds of files) would otherwise abort this - # whole while-loop subshell -- another way to reintroduce the "no - # candidate found" failure the ${mtime:-0} fallback below already exists to - # avoid. - find "$dir" -type f -name 'rollout-*.jsonl' 2>/dev/null | while IFS= read -r f; do - mtime=$(compat_file_mtime "$f" || true) - printf '%s\t%s\n' "${mtime:-0}" "$f" - done | sort -t "$(printf '\t')" -k1,1rn | awk -F'\t' -v n="$limit" 'NR<=n { sub(/^[^\t]*\t/, ""); print }' + # + # A rollout that `find` listed but that Codex deletes or rotates before + # the stat batch reaches it no longer needs special handling here: + # compat_files_mtime_0 suppresses the per-file error and still prints + # every surviving file, so a dead file can't starve the caller of the + # whole candidate list. + find "$dir" -type f -name 'rollout-*.jsonl' -print0 2>/dev/null \ + | compat_files_mtime_0 \ + | sort -t "$(printf '\t')" -k1,1rn | awk -F'\t' -v n="$limit" 'NR<=n { sub(/^[^\t]*\t/, ""); print }' } # Resolve the current Codex thread id. CODEX_THREAD_ID is only exported on the diff --git a/scripts/lib/compat.sh b/scripts/lib/compat.sh index 883ed04f3..156f84446 100644 --- a/scripts/lib/compat.sh +++ b/scripts/lib/compat.sh @@ -219,3 +219,32 @@ compat_file_mtime() { *) stat -c %Y "$file" 2>/dev/null ;; esac } + +# Batch variant of compat_file_mtime: read NUL-separated paths on stdin +# (find -print0) and emit one "" line per file, spawning +# one stat per argv batch instead of one process chain per file. +# +# Why a batch form exists at all: `mtime=$(compat_file_mtime "$f")` in a +# per-file loop pays a command-substitution subshell + uname + stat for +# every file (the subshell also discards the platform memo each time). On +# Windows/MSYS2, where process creation is orders of magnitude costlier +# than a Linux fork, that turns a scan of a few thousand files into +# minutes of wall clock. xargs keeps each stat invocation under the argv +# limit (cf. the E2BIG failures in #882), so arbitrarily long lists stay +# safe. +# +# A path that vanishes between the listing and the stat batch (a real race +# against a live writer) makes stat report an error for that file and exit +# nonzero, but every surviving file is still printed -- so the error is +# suppressed and the status discarded rather than letting one dead file +# starve the caller of the whole list. Empty input is likewise quiet: stat +# with no operands fails, which the same suppression covers. +compat_files_mtime_0() { + _agmsg_detect_platform + local tab + tab=$(printf '\t') + case "$_agmsg_platform" in + macos) xargs -0 stat -f "%m${tab}%N" 2>/dev/null ;; + *) xargs -0 stat -c "%Y${tab}%n" 2>/dev/null ;; + esac || true +} From a8c7d28d1deaf8938d02cbfb912cbc666c0fc2d9 Mon Sep 17 00:00:00 2001 From: hiroyuki117339 Date: Sun, 13 Sep 2026 08:57:06 +0900 Subject: [PATCH 08/34] fix(codex): discover a ws:// app-server in the SessionStart plug (#1057) The SessionStart plug looked for the app-server in three places -- the AGMSG_CODEX_BRIDGE_APP_SERVER variable, a unix:// token on the agent cmdline, and a .sock file -- and exited when none matched. A ws:// app-server matches none of them: codex-monitor.sh hands the URL to codex --remote and records the port in run/codex-app-server..port. The plug therefore never wrote codex-bridge-request., the launcher kept whatever that file last held, and every relaunch was pinned to a dead port with no self-recovery (#1056, first half). Add the port file as a last-resort probe, through the existing _agmsg_codex_app_server_url helper in _app-server.sh, after the three probes that already exist. The helper returns empty when no port can be resolved, so the genuine "no app-server" case still exits as before. The second half of #1056 -- the launcher trusting the request file without checking that the endpoint is reachable -- is a separate design decision and is not addressed here. --- scripts/drivers/types/codex/_session-start.sh | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/scripts/drivers/types/codex/_session-start.sh b/scripts/drivers/types/codex/_session-start.sh index 9375ba38f..9530cb1bb 100644 --- a/scripts/drivers/types/codex/_session-start.sh +++ b/scripts/drivers/types/codex/_session-start.sh @@ -144,6 +144,17 @@ agmsg_session_start() { app_server="unix://$socket_path" fi fi + if [ -z "$app_server" ]; then + # A ws:// (TCP) app-server has no socket file to find, and codex-monitor.sh + # passes the URL to `codex --remote`, not as a `unix://` token this script can + # scrape — so none of the three probes above can see it. The port file does + # carry the URL; reuse the helper codex-record-session.sh already uses for it. + if ! command -v _agmsg_codex_app_server_url >/dev/null 2>&1; then + # shellcheck disable=SC1091 + . "$SKILL_DIR/scripts/drivers/types/codex/_app-server.sh" + fi + app_server="$(_agmsg_codex_app_server_url "$PROJECT")" + fi [ -n "$app_server" ] || exit 0 if [ "${AGMSG_CODEX_BRIDGE_LAUNCHER:-}" = "1" ]; then From 99a3e9e26f758b7a46d6e8fa88c5a731017eddd6 Mon Sep 17 00:00:00 2001 From: fujibee Date: Sat, 12 Sep 2026 17:37:42 -0700 Subject: [PATCH 09/34] ci: record per-file bats timings (#1159) * ci: record per-file bats timings * test: preserve timings when a shard is cancelled * test: make timing summary assertions enforceable * fix(ci): isolate timed bats input from the shard manifest * test(ci): keep the shard manifest off standard input --- .github/scripts/run-bats-timed.sh | 78 +++++++++++++ .github/scripts/shard-tests.sh | 3 + .github/scripts/summarize-bats-timings.sh | 56 +++++++++ .github/workflows/tests.yml | 14 ++- tests/test_ci_sharding.bats | 132 ++++++++++++++++++++++ 5 files changed, 282 insertions(+), 1 deletion(-) create mode 100755 .github/scripts/run-bats-timed.sh create mode 100755 .github/scripts/summarize-bats-timings.sh diff --git a/.github/scripts/run-bats-timed.sh b/.github/scripts/run-bats-timed.sh new file mode 100755 index 000000000..e55013942 --- /dev/null +++ b/.github/scripts/run-bats-timed.sh @@ -0,0 +1,78 @@ +#!/usr/bin/env bash +# Run one bats file at a time and append machine-readable wall-clock timings. +# +# Usage: run-bats-timed.sh +# +# The TSV is intentionally append-only while the suite runs. If the job reaches +# its wall-clock cap, every completed file remains useful evidence instead of +# disappearing with the unfinished bats invocation. Files from several runs can +# be concatenated directly: the run metadata is repeated on every row. +set -u + +usage() { + echo "usage: ${0##*/} " >&2 + exit 2 +} + +[ "$#" -eq 2 ] || usage +manifest="$1" +timings="$2" +[ -s "$manifest" ] || { echo "${0##*/}: empty or missing manifest: $manifest" >&2; exit 1; } + +run_id="${GITHUB_RUN_ID:-local}" +run_attempt="${GITHUB_RUN_ATTEMPT:-1}" +sha="${GITHUB_SHA:-unknown}" +runner_os="${RUNNER_OS:-unknown}" +shard="${SHARD:-unknown}" +shard_total="${SHARD_TOTAL:-unknown}" + +printf 'schema\trecord\trun_id\trun_attempt\tsha\tos\tshard\tshard_total\tfile\tstarted_at\tended_at\telapsed_seconds\tstatus\n' > "$timings" + +suite_started_epoch="$(date -u +%s)" +suite_started_at="$(date -u +%Y-%m-%dT%H:%M:%SZ)" +suite_status=0 +completed=0 + +finish() { + rc=$? + suite_ended_epoch="$(date -u +%s)" + suite_ended_at="$(date -u +%Y-%m-%dT%H:%M:%SZ)" + suite_elapsed=$((suite_ended_epoch - suite_started_epoch)) + [ "$suite_status" -ne 0 ] || suite_status="$rc" + printf '1\tshard\t%s\t%s\t%s\t%s\t%s\t%s\t-\t%s\t%s\t%s\t%s\n' \ + "$run_id" "$run_attempt" "$sha" "$runner_os" "$shard" "$shard_total" \ + "$suite_started_at" "$suite_ended_at" "$suite_elapsed" "$suite_status" >> "$timings" + echo "bats timing: shard $shard/$shard_total completed $completed file(s) in ${suite_elapsed}s (status $suite_status)" +} +trap finish EXIT +stop() { + suite_status=143 + exit 143 +} +trap stop INT TERM + +while IFS= read -r file <&3; do + [ -n "$file" ] || continue + started_epoch="$(date -u +%s)" + started_at="$(date -u +%Y-%m-%dT%H:%M:%SZ)" + printf '1\tfile_start\t%s\t%s\t%s\t%s\t%s\t%s\t%s\t%s\t-\t-\t-\n' \ + "$run_id" "$run_attempt" "$sha" "$runner_os" "$shard" "$shard_total" \ + "$file" "$started_at" >> "$timings" + echo "bats timing: start $file at $started_at" + + bats --print-output-on-failure "$file" > "$timings" + echo "bats timing: end $file at $ended_at (${elapsed}s, status $status)" + + completed=$((completed + 1)) + [ "$status" -eq 0 ] || suite_status="$status" +done 3< "$manifest" + +exit "$suite_status" diff --git a/.github/scripts/shard-tests.sh b/.github/scripts/shard-tests.sh index 13db2f598..e6dcba9da 100755 --- a/.github/scripts/shard-tests.sh +++ b/.github/scripts/shard-tests.sh @@ -22,6 +22,9 @@ # come out at 125s/298s/366s/71s. Per-test cost varies from ~0.0s to ~8s # depending on how much a file forks or waits. So the real speedup here is # 860s -> 366s (~2.4x), not 4x. +# These are historical design measurements, not current weights. Every CI shard +# now uploads a bats-timings artifact with per-file and shard wall times; combine +# several with summarize-bats-timings.sh before changing the partition. # # It is still the right weight to ship first. The alternative, a checked-in # table of measured per-file seconds, buys ~150s more but goes stale silently: diff --git a/.github/scripts/summarize-bats-timings.sh b/.github/scripts/summarize-bats-timings.sh new file mode 100755 index 000000000..3edfbb353 --- /dev/null +++ b/.github/scripts/summarize-bats-timings.sh @@ -0,0 +1,56 @@ +#!/usr/bin/env bash +# Summarize one or more run-bats-timed.sh TSV artifacts. +# +# Usage: summarize-bats-timings.sh [--timeout-seconds N] ... +# +# File percentiles are nearest-rank values across every supplied run. Shard +# headroom is kept per run and OS so a fast sample cannot hide a slow sibling. +set -euo pipefail + +timeout=1800 +if [ "${1:-}" = --timeout-seconds ]; then + timeout="${2:-}" + shift 2 +fi +case "$timeout" in ''|*[!0-9]*) echo "${0##*/}: timeout must be seconds" >&2; exit 2 ;; esac +[ "$#" -gt 0 ] || { echo "usage: ${0##*/} [--timeout-seconds N] ..." >&2; exit 2; } + +tmp="$(mktemp -d "${TMPDIR:-/tmp}/agmsg-bats-timings.XXXXXX")" +cleanup() { + rm -f "$tmp/files" "$tmp/shards" + rmdir "$tmp" 2>/dev/null || true +} +trap cleanup EXIT + +awk -F '\t' '$1 == 1 && $2 == "file_end" && $13 == 0 { print $6 "\t" $9 "\t" $12 }' "$@" \ + | LC_ALL=C sort -t ' ' -k1,1 -k2,2 -k3,3n > "$tmp/files" + +printf 'record\tos\tfile\tsamples\tp50_seconds\tp95_seconds\tmax_seconds\n' +awk -F '\t' ' + function emit( p50,p95) { + if (!n) return + p50 = int((n + 1) / 2) + p95 = int((95 * n + 99) / 100) + printf "file\t%s\t%s\t%d\t%d\t%d\t%d\n", os, file, n, value[p50], value[p95], value[n] + } + { + key = $1 FS $2 + if (last != "" && key != last) { emit(); delete value; n=0 } + os=$1; file=$2; value[++n]=$3; last=key + } + END { emit() } +' "$tmp/files" + +awk -F '\t' '$1 == 1 && $2 == "shard" { print $3 "\t" $4 "\t" $6 "\t" $7 "\t" $12 "\t" $13 }' "$@" \ + | LC_ALL=C sort -t ' ' -k1,1 -k2,2n -k3,3 -k5,5nr > "$tmp/shards" + +printf 'record\trun_id\trun_attempt\tos\tmax_shard\tmax_seconds\theadroom_seconds\tstatus\n' +awk -F '\t' -v timeout="$timeout" ' + { + key=$1 FS $2 FS $3 + if (!(key in seen)) { + seen[key]=1 + printf "run\t%s\t%s\t%s\t%s\t%s\t%s\t%s\n", $1, $2, $3, $4, $5, timeout-$5, $6 + } + } +' "$tmp/shards" diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index 3aee94ebb..f8438336f 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -391,9 +391,11 @@ jobs: - name: Run bats suite (this shard) if: needs.changes.outputs.docs_only != 'true' + env: + SHARD: ${{ matrix.shard }} run: | set +e - xargs bats --print-output-on-failure < shard-files.txt + .github/scripts/run-bats-timed.sh shard-files.txt "$RUNNER_TEMP/bats-timings.tsv" status=$? : > "$RUNNER_TEMP/bats-done" exit $status @@ -451,6 +453,16 @@ jobs: if-no-files-found: ignore retention-days: 3 + - name: Upload bats timings + if: always() && needs.changes.outputs.docs_only != 'true' + continue-on-error: true + uses: actions/upload-artifact@v4 + with: + name: bats-timings-${{ matrix.os }}-${{ matrix.shard }} + path: ${{ runner.temp }}/bats-timings.tsv + if-no-files-found: warn + retention-days: 30 + - name: Record which files this shard ran if: needs.changes.outputs.docs_only != 'true' uses: actions/upload-artifact@v4 diff --git a/tests/test_ci_sharding.bats b/tests/test_ci_sharding.bats index 98bf510b0..d1766f9b8 100644 --- a/tests/test_ci_sharding.bats +++ b/tests/test_ci_sharding.bats @@ -9,6 +9,25 @@ setup() { load 'test_helper' REPO_ROOT="$(cd "$BATS_TEST_DIRNAME/.." && pwd)" SHARD="$REPO_ROOT/.github/scripts/shard-tests.sh" + TIMED_RUNNER="$REPO_ROOT/.github/scripts/run-bats-timed.sh" + TIMING_SUMMARY="$REPO_ROOT/.github/scripts/summarize-bats-timings.sh" +} + +@test "timing summary computes cross-run percentiles and per-run headroom" { + local timings + timings="$BATS_TEST_TMPDIR/timings.tsv" + printf '%s\n' \ + $'schema\trecord\trun_id\trun_attempt\tsha\tos\tshard\tshard_total\tfile\tstarted_at\tended_at\telapsed_seconds\tstatus' \ + $'1\tfile_end\t1\t1\ta\tmacOS\t1\t4\ttests/a.bats\ts\te\t10\t0' \ + $'1\tfile_end\t2\t1\tb\tmacOS\t2\t4\ttests/a.bats\ts\te\t30\t0' \ + $'1\tshard\t1\t1\ta\tmacOS\t1\t4\t-\ts\te\t70\t0' \ + $'1\tshard\t1\t1\ta\tmacOS\t2\t4\t-\ts\te\t90\t0' > "$timings" + + run "$TIMING_SUMMARY" --timeout-seconds 100 "$timings" + + [ "$status" -eq 0 ] + grep -Fq $'file\tmacOS\ttests/a.bats\t2\t10\t30\t30' <<< "$output" + grep -Fq $'run\t1\t1\tmacOS\t2\t90\t10\t0' <<< "$output" } all_test_files() { @@ -29,6 +48,119 @@ union_of_shards() { [[ "$output" == *"usage:"* ]] } +@test "timed bats runner is executable and self-documents its usage" { + [ -x "$TIMED_RUNNER" ] + run "$TIMED_RUNNER" + [ "$status" -eq 2 ] + [[ "$output" == *"usage:"* ]] +} + +@test "timed bats runner records each file and the shard total" { + local fixture manifest timings fake_bin + fixture="$BATS_TEST_TMPDIR/fixture.bats" + manifest="$BATS_TEST_TMPDIR/manifest.txt" + timings="$BATS_TEST_TMPDIR/timings.tsv" + fake_bin="$BATS_TEST_TMPDIR/bin" + mkdir -p "$fake_bin" + printf '@test "passes" { true; }\n' > "$fixture" + printf '%s\n' "$fixture" > "$manifest" + printf '#!/usr/bin/env bash\nexit 0\n' > "$fake_bin/bats" + chmod +x "$fake_bin/bats" + + run env PATH="$fake_bin:$PATH" GITHUB_RUN_ID=42 GITHUB_RUN_ATTEMPT=3 \ + GITHUB_SHA=abc RUNNER_OS=macOS SHARD=2 SHARD_TOTAL=4 \ + "$TIMED_RUNNER" "$manifest" "$timings" + + [ "$status" -eq 0 ] + [ "$(awk -F '\t' '$2 == "file_start" { n++ } END { print n+0 }' "$timings")" -eq 1 ] + [ "$(awk -F '\t' '$2 == "file_end" { n++ } END { print n+0 }' "$timings")" -eq 1 ] + [ "$(awk -F '\t' '$2 == "shard" { n++ } END { print n+0 }' "$timings")" -eq 1 ] + awk -F '\t' '$2 == "file_end" && $3 == 42 && $4 == 3 && $5 == "abc" && $6 == "macOS" && $7 == 2 && $8 == 4 && $9 != "" && $12 ~ /^[0-9]+$/ && $13 == 0 { ok=1 } END { exit !ok }' "$timings" +} + +@test "timed bats runner records a failure and continues the shard" { + local manifest timings fake_bin calls + manifest="$BATS_TEST_TMPDIR/manifest.txt" + timings="$BATS_TEST_TMPDIR/timings.tsv" + fake_bin="$BATS_TEST_TMPDIR/bin" + calls="$BATS_TEST_TMPDIR/calls.txt" + mkdir -p "$fake_bin" + printf '%s\n' tests/fail.bats tests/pass.bats > "$manifest" + printf '%s\n' '#!/usr/bin/env bash' 'echo "$2" >> "$BATS_CALLS"' 'case "$2" in *fail*) exit 7 ;; esac' > "$fake_bin/bats" + chmod +x "$fake_bin/bats" + + run env PATH="$fake_bin:$PATH" BATS_CALLS="$calls" "$TIMED_RUNNER" "$manifest" "$timings" + + [ "$status" -eq 7 ] + [ "$(wc -l < "$calls" | tr -d ' ')" -eq 2 ] + [ "$(awk -F '\t' '$2 == "file_end" { n++ } END { print n+0 }' "$timings")" -eq 2 ] + awk -F '\t' '$2 == "shard" && $13 == 7 { ok=1 } END { exit !ok }' "$timings" +} + +@test "timed bats runner does not pass the manifest as test stdin" { + local manifest timings fake_bin calls stdin_capture + manifest="$BATS_TEST_TMPDIR/manifest.txt" + timings="$BATS_TEST_TMPDIR/timings.tsv" + fake_bin="$BATS_TEST_TMPDIR/bin" + calls="$BATS_TEST_TMPDIR/calls.txt" + stdin_capture="$BATS_TEST_TMPDIR/stdin.txt" + mkdir -p "$fake_bin" + printf '%s\n' tests/first.bats tests/second.bats > "$manifest" + printf '%s\n' \ + '#!/usr/bin/env bash' \ + 'echo "$2" >> "$BATS_CALLS"' \ + 'cat >> "$BATS_STDIN_CAPTURE"' \ + 'exit 0' > "$fake_bin/bats" + chmod +x "$fake_bin/bats" + + run env PATH="$fake_bin:$PATH" BATS_CALLS="$calls" BATS_STDIN_CAPTURE="$stdin_capture" \ + "$TIMED_RUNNER" "$manifest" "$timings" + + [ "$status" -eq 0 ] + [ "$(wc -l < "$calls" | tr -d ' ')" -eq 2 ] + [ ! -s "$stdin_capture" ] +} + +@test "timed bats runner records the interrupted shard on TERM" { + local manifest timings fake_bin ready release runner_pid rc i + manifest="$BATS_TEST_TMPDIR/manifest.txt" + timings="$BATS_TEST_TMPDIR/timings.tsv" + fake_bin="$BATS_TEST_TMPDIR/bin" + ready="$BATS_TEST_TMPDIR/ready" + release="$BATS_TEST_TMPDIR/release" + mkdir -p "$fake_bin" + printf '%s\n' tests/running.bats > "$manifest" + printf '%s\n' '#!/usr/bin/env bash' ': > "$BATS_READY"' 'while [ ! -f "$BATS_RELEASE" ]; do sleep 0.05; done' > "$fake_bin/bats" + chmod +x "$fake_bin/bats" + + PATH="$fake_bin:$PATH" BATS_READY="$ready" BATS_RELEASE="$release" \ + "$TIMED_RUNNER" "$manifest" "$timings" > "$BATS_TEST_TMPDIR/runner.log" 2>&1 & + runner_pid=$! + i=0 + while [ ! -f "$ready" ] && [ "$i" -lt 100 ]; do + sleep 0.05 + i=$((i + 1)) + done + [ -f "$ready" ] + + kill -TERM "$runner_pid" + : > "$release" + rc=0 + wait "$runner_pid" || rc=$? + + [ "$rc" -eq 143 ] + [ "$(awk -F '\t' '$2 == "file_start" { n++ } END { print n+0 }' "$timings")" -eq 1 ] + awk -F '\t' '$2 == "shard" && $13 == 143 { ok=1 } END { exit !ok }' "$timings" +} + +@test "CI runs the timed runner and uploads each shard artifact" { + local workflow + workflow="$REPO_ROOT/.github/workflows/tests.yml" + grep -Fq '.github/scripts/run-bats-timed.sh shard-files.txt "$RUNNER_TEMP/bats-timings.tsv"' "$workflow" + grep -Fq 'name: bats-timings-${{ matrix.os }}-${{ matrix.shard }}' "$workflow" + grep -Fq 'path: ${{ runner.temp }}/bats-timings.tsv' "$workflow" +} + @test "the shards cover every test file exactly once" { # Checked across several totals: an off-by-one in the greedy loop can easily # be invisible at one shard count and drop a file at another. From ebb6b61f917ef5a8b8c4e11a6b3029111315918f Mon Sep 17 00:00:00 2001 From: fujibee Date: Sat, 12 Sep 2026 18:37:57 -0700 Subject: [PATCH 10/34] test(codex): pin the ws:// port-file fallback in the SessionStart plug (#1056) (#1172) Adds coverage for the fourth app-server discovery fallback introduced in #1057: with AGMSG_CODEX_BRIDGE_APP_SERVER unset, no unix:// token on the agent cmdline, and no .sock file, the SessionStart plug still finds a ws:// app-server through its port file and writes the codex-bridge-request with that URL. Measured red on the commit immediately before #1057 landed on main and green from #1057 onward. Refs #1056. --- tests/test_delivery.bats | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/tests/test_delivery.bats b/tests/test_delivery.bats index 6d4e4a7c2..c0cd7808c 100644 --- a/tests/test_delivery.bats +++ b/tests/test_delivery.bats @@ -2306,6 +2306,30 @@ EOF grep -q -- "--inline-inbox" "$log" } +@test "session-start.sh for codex writes the bridge request from a ws:// port file alone (#1056)" { + # No AGMSG_CODEX_BRIDGE_APP_SERVER, no unix:// token on the agent's cmdline + # (AGMSG_AGENT_PID is "" per setup()), and no .sock file -- the first three + # app-server probes all come up empty. Only the port file _app-server.sh's + # _agmsg_codex_app_server_url reads is present, carrying a ws:// port. + bash "$SCRIPTS/join.sh" team alice codex "$TEST_PROJECT" >/dev/null + _seed_role_record team alice thread-ws-1056 "$TEST_PROJECT" codex + + # shellcheck disable=SC1091 + source "$SCRIPTS/lib/hash.sh" + local hash; hash="$(printf '%s' "$TEST_PROJECT" | agmsg_sha1)" + mkdir -p "$TEST_SKILL_DIR/run" + printf '50505' > "$TEST_SKILL_DIR/run/codex-app-server.$hash.port" + + ( unset AGMSG_CODEX_BRIDGE_APP_SERVER + AGMSG_CODEX_BRIDGE_LAUNCHER=1 \ + CODEX_THREAD_ID="thread-ws-1056" \ + bash "$SCRIPTS/session-start.sh" codex "$TEST_PROJECT" >/dev/null ) + + local request_file="$TEST_SKILL_DIR/run/codex-bridge-request.$hash" + [ -f "$request_file" ] + grep -q -- "ws://127.0.0.1:50505" "$request_file" +} + @test "session-start.sh for codex stays quiet without monitor launcher env" { bash "$SCRIPTS/join.sh" team alice codex "$TEST_PROJECT" >/dev/null local fake="$TEST_SKILL_DIR/fake-codex-bridge" From a4fc81ff99c8f1936d72063cdbc9c5f424e93f42 Mon Sep 17 00:00:00 2001 From: fujibee Date: Sat, 12 Sep 2026 21:32:47 -0700 Subject: [PATCH 11/34] ci(tests): seed pinned bats files by measured cost, not @test count (#847) (#1175) The pinned-apart heavy files (#847/#848) were seeded into their shards at their @test count (file_weight): refusal 9, liveness 31. That count is exactly the metric these files defeat -- both wait far more than they compute -- so a 9-unit seed left refusal's shard looking almost empty and the greedy LPT pass piled an average count-share of other files on top of an already-679s file. Measured on the first green run to record per-file timings (#1159), that overfilled shard ran 1475s while the others ran 636-737s. Seed each pinned file instead at what its measured wall time is worth in average tests: round(measured macOS wall / whole-suite avg s/test). Off run 34664794167 (4212s over 1749 @tests => 2.408 s/test) that is refusal 282, liveness 122. Simulating the partition against the measured per-file seconds (the simulator reproduces the observed 1475s under the old seed, so it is trustworthy) brings the worst shard from 1475s to 987s. The seed is tied to each file's OWN measured cost rather than to a count the suite outgrows, so it does not drift as the suite grows -- which is what #1107 actually was, a partition sized against a count baseline left behind as the suite roughly tripled. The header records what the values were measured against and that numerator and divisor must be refreshed together. Placement is unchanged (slot % total), so the pins still land in distinct shards; tests/test_ci_sharding.bats stays green (11/11). --- .github/scripts/shard-tests.sh | 61 ++++++++++++++++++++++++++++------ tests/test_ci_sharding.bats | 2 +- 2 files changed, 51 insertions(+), 12 deletions(-) diff --git a/.github/scripts/shard-tests.sh b/.github/scripts/shard-tests.sh index e6dcba9da..d6549fdf8 100755 --- a/.github/scripts/shard-tests.sh +++ b/.github/scripts/shard-tests.sh @@ -48,16 +48,18 @@ # per-test cost -- is NOT this case; count already weights it correctly, and # it is not pinned. # -# Measured 2026-08-19 on a green main run (head 626a625b, run 32193147987) by -# correlating each `ok N ` line's own GitHub Actions timestamp against -# which file's `@test` block that description belongs to, then ranking every -# file by seconds-per-test rather than by raw duration (raw duration alone -# does not distinguish "slow because few tests wait a long time" from "slow -# because there are simply many tests", and only the former is what count -# weighting misses): +# First ranked 2026-08-19 by correlating each `ok N ` line's own GitHub +# Actions timestamp against which file's `@test` block that description belongs +# to, then ranking every file by seconds-per-test rather than by raw duration +# (raw duration alone does not distinguish "slow because few tests wait a long +# time" from "slow because there are simply many tests", and only the former is +# what count weighting misses). Re-measured 2026-09-12 from the per-file wall +# times that run-bats-timed.sh now records (#1159), read off the green run that +# first carried them (GitHub Actions run 34664794167, tests.yml bats job, +# macos-latest, 5 shards): # -# tests/test_remote_engine_start_refusal.bats 722s / 9 tests = ~80s/test -# tests/test_remote_status_liveness.bats 380s / 31 tests = ~12s/test +# tests/test_remote_engine_start_refusal.bats 679s / 9 tests = ~75s/test +# tests/test_remote_status_liveness.bats 293s / 31 tests = ~9s/test # # against a whole-suite per-test cost this script's own header already says # runs ~0.0s-8s. Both are 1.5x-10x above that ceiling on a low test count, so @@ -77,7 +79,7 @@ # theoretical one. # # This does not bound a shard's total duration: the heavier entry above -# (722s) is heavy enough on its own that no repacking of the rest of the +# (679s) is heavy enough on its own that no repacking of the rest of the # suite moves its shard's floor by much. See tests.yml's bats-shard # timeout-minutes for the ceiling this is paired with, sized to cover that # floor plus a fair share of everything else with real margin. And a file NOT @@ -139,6 +141,43 @@ file_weight() { printf '%s' "$n" } +# Load a pinned file contributes when it is seeded, in the same unit the +# weighted pass below uses: @test-equivalents. A pinned file's @test count +# (its file_weight) badly understates its cost -- that is the whole reason it +# is pinned -- so seeding it at that count leaves its shard looking almost +# empty and the greedy pass piles an average share of other files on top of an +# already-expensive file. Seed it instead at what its measured wall time is +# WORTH in average tests: round(measured_macOS_wall_seconds / avg_s_per_test). +# +# This is tied to the file's OWN measured cost, not to a count the suite +# outgrows, so it does not drift as the suite grows -- which is exactly the +# failure #1107 was (a partition sized against a count baseline that went +# stale as the suite roughly tripled). +# +# MEASURED AGAINST: GitHub Actions run 34664794167 (#1159, tests.yml bats +# job, macos-latest, 5 shards, 2026-09-12). Whole-suite macOS wall summed +# over every file = 4212s across 1749 @test blocks => avg = 4212 / 1749 = +# 2.408 s/test. Both seeds below are that run's per-file wall / that avg: +# test_remote_engine_start_refusal.bats 679s / 2.408 = round(281.9) = 282 +# test_remote_status_liveness.bats 293s / 2.408 = round(121.7) = 122 +# +# The numerator (per-file wall) and the divisor (whole-suite avg) came from the +# SAME run, so a refresh must re-derive BOTH together off one green run's +# bats-timings artifacts (summarize-bats-timings.sh) -- never update one alone. +# A pinned file with no measured seed here falls back to its @test count. +# Exact string equality, not `case`: a bare `case` glob widens under an +# inherited `nocasematch`/locale, and these are meant to match one literal +# basename each and nothing else. +pin_seed() { + if [ "$1" = test_remote_engine_start_refusal.bats ]; then + printf 282 + elif [ "$1" = test_remote_status_liveness.bats ]; then + printf 122 + else + file_weight "$2" + fi +} + # Seed the pinned files into distinct shards first, in PINNED_APART's own # (measured-heaviest-first) order — not the order they happen to sort in # below, which is by count and is exactly the metric these files defeat. Each @@ -166,7 +205,7 @@ $files EOF [ -n "$match" ] || continue s=$((slot % total)) - load[s]=$((load[s] + $(file_weight "$match"))) + load[s]=$((load[s] + $(pin_seed "$p" "$match"))) if [ "$s" -eq "$((index - 1))" ]; then printf '%s\n' "$match" fi diff --git a/tests/test_ci_sharding.bats b/tests/test_ci_sharding.bats index d1766f9b8..11e7fa977 100644 --- a/tests/test_ci_sharding.bats +++ b/tests/test_ci_sharding.bats @@ -220,7 +220,7 @@ union_of_shards() { # The test above catches drift in the TOP TWO BY COUNT — exactly the metric # that misses the files pinned below (#847, #848): both are near the bottom # of the count-weighted sort (9 and 31 tests) despite carrying some of the -# largest measured durations in the suite (722s and 380s; see +# largest measured durations in the suite (679s and 293s; see # shard-tests.sh's own comment for the measurement). This test guards the # actual fix, not the metric that already worked. @test "the pinned-apart heavy files never share a shard, at any shard total >= 2 (#847, #848)" { From 4851c630c54ca26646432db1e5ca9c0c064bece8 Mon Sep 17 00:00:00 2001 From: AI nyan Date: Sun, 13 Sep 2026 13:52:14 +0900 Subject: [PATCH 12/34] =?UTF-8?q?fix(codex-bridge):=20one=20wake,=20one=20?= =?UTF-8?q?turn=20=E2=80=94=20attribute=20mid-start=20turn-end=20signals?= =?UTF-8?q?=20by=20turn=20id=20(#889)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit One wake could start two turns: a turn-end signal landing while the turn/start request was still unanswered re-entered tryStartTurn() with the same wake, and the second turn was an empty poll that burned a turn on the live thread (#870). Claim the wake before the request goes out, gate turn-end signals while the request is in flight, and attribute mid-start end signals by turn id so a fast turn that is fully notified before its ACK still ends promptly. The early wake claim is kept deliberately: a new wake arriving from the watch poll during the request must not be collapsed into the consumed one. Signals without a turn id are dropped only while a start is in flight; afterwards the existing idle watchdog converges as before. Fixes #870. --- scripts/drivers/types/codex/codex-bridge.js | 73 ++++- tests/test_codex_bridge.bats | 294 ++++++++++++++++++++ 2 files changed, 363 insertions(+), 4 deletions(-) diff --git a/scripts/drivers/types/codex/codex-bridge.js b/scripts/drivers/types/codex/codex-bridge.js index 9cda16b3d..da4165934 100755 --- a/scripts/drivers/types/codex/codex-bridge.js +++ b/scripts/drivers/types/codex/codex-bridge.js @@ -940,6 +940,9 @@ class CodexBridge { this.turnActive = false; this.turnTimer = null; this.pendingWake = false; + this.startInFlight = false; + this.inFlightTurnId = null; + this.inFlightTurnEnded = false; this.watchHandle = null; this.wakeCount = 0; this.lastWakeMaxId = ""; @@ -980,7 +983,18 @@ class CodexBridge { this.client.on("error", this.clientHandler("error", (params) => this.onServerError(params))); this.client.on("item/agentMessage/delta", this.clientHandler("item/agentMessage/delta", (params) => this.onAgentMessageDelta(params))); this.client.on("thread/status/changed", this.clientHandler("thread/status/changed", (params) => this.onThreadStatus(params))); - this.client.on("turn/started", this.clientHandler("turn/started", () => { + this.client.on("turn/started", this.clientHandler("turn/started", (params) => { + // The app-server holds threads beyond ours; another thread's turn must + // not flip our state (and, below, must not be mistaken for the turn we + // are starting). + if (params && params.threadId && params.threadId !== this.threadId) return; + // The app-server may notify the turn tryStartTurn() is starting BEFORE + // it ACKs the turn/start request. Capture its IDENTITY: only an end + // signal carrying this same turn id may be attributed to the new turn + // while the request is in flight (see onTurnCompleted). + if (this.startInFlight) { + this.inFlightTurnId = (params && params.turn && params.turn.id) || null; + } this.turnActive = true; this.threadIdle = false; // This turn was not started by tryStartTurn() -- e.g. a TUI-driven turn @@ -1395,7 +1409,10 @@ class CodexBridge { return; } if (type === "idle") { - this.threadIdle = true; + // While a turn/start request is in flight, that start owns the state; + // a stale idle from the previous turn must not flip threadIdle under + // it. onTurnEnded() below decides (and defers) via the same ownership. + if (!this.startInFlight) this.threadIdle = true; // The real app-server signals idle but may never send turn/completed; // treat idle as the end of the turn so detection resumes. See #41. this.onTurnEnded().catch((error) => @@ -1411,6 +1428,21 @@ class CodexBridge { } else { console.error(`codex-bridge: turn completed on thread ${this.threadId}`); } + // Attribution while our turn/start request is unanswered. The previous + // turn's tail and the NEW turn's own completion are both legal here, and + // a phase flag cannot tell them apart (a stale tail can land AFTER the + // new turn was seen starting). Identity can: defer the end only when it + // carries the SAME turn id turn/started reported for the turn we are + // starting. Anything else — a different id, or no id on either side — is + // unattributable mid-start and is dropped; if it really was the new + // turn's end, the idle watchdog closes the turn (#41). + if (this.startInFlight) { + const completedId = params.turn && params.turn.id; + if (completedId && this.inFlightTurnId && completedId === this.inFlightTurnId) { + this.inFlightTurnEnded = true; + } + return; + } await this.onTurnEnded(); } @@ -1419,6 +1451,18 @@ class CodexBridge { // real app-server does not reliably deliver turn/completed, so a bridge that // gates re-arm on it never re-arms and sleeps after one message. See #41. async onTurnEnded() { + // While our turn/start request is unanswered, the only turn-end signal + // that can be attributed to the turn being started is an id-matching + // turn/completed — and onTurnCompleted defers that one itself before it + // ever reaches here. Everything else that funnels in mid-start (a stale + // thread/status idle from the previous turn, an id-less completion, a + // watchdog firing) is unattributable: acting on it reset turnActive / + // threadIdle under the in-flight start and re-entered tryStartTurn with + // the same wake, injecting a duplicate turn whose inbox read — after the + // first read consumed the rows — was empty. Drop them; a genuinely-ended + // new turn that only signalled ambiguously is closed by the idle + // watchdog (#41). + if (this.startInFlight) return; this.clearTurnWatchdog(); this.turnActive = false; this.threadIdle = true; @@ -1456,6 +1500,16 @@ class CodexBridge { const prompt = this.buildPrompt(); this.turnActive = true; this.threadIdle = false; + // Claim the wake BEFORE the request goes out, not after it succeeds. With + // the claim left set across the await, a turn-end signal arriving mid- + // request re-entered this method with the same wake and started a second + // turn. The claim is restored on failure so the wake fires again (the + // inline inbox rows are already marked read by then, so the retry + // re-delivers the wake, not the payload — unchanged from before). + this.pendingWake = false; + this.startInFlight = true; + this.inFlightTurnId = null; + this.inFlightTurnEnded = false; try { await this.client.request("turn/start", { threadId: this.threadId, @@ -1464,16 +1518,24 @@ class CodexBridge { runtimeWorkspaceRoots: this.opts.workspaceRoots, }); console.error(`codex-bridge: started turn on thread ${this.threadId}`); - this.pendingWake = false; // Bound how long we treat the turn as active. The real app-server may // never send turn/completed; the watchdog (and thread/status idle) drive // onTurnEnded so detection re-arms instead of sleeping forever. See #41. this.startTurnWatchdog(); } catch (error) { + this.pendingWake = true; this.turnActive = false; this.threadIdle = true; this.clearTurnWatchdog(); throw error; + } finally { + this.startInFlight = false; + } + // A fast turn can be fully notified (started AND ended) before the ACK + // arrived; its deferred end is processed now that the start is settled. + if (this.inFlightTurnEnded) { + this.inFlightTurnEnded = false; + await this.onTurnEnded(); } } @@ -1583,7 +1645,10 @@ class CodexBridge { const sections = []; for (const pair of this.identities) { if (!allowed.has(`${pair.team}\t${pair.name}`)) continue; - const result = spawnSync(BASH_BIN, [path.join(SCRIPTS_DIR, "inbox.sh"), pair.team, pair.name], { cwd: this.opts.project, encoding: "utf8" }); + // --quiet: an empty inbox must read back as EMPTY. The human-facing + // "No new messages." line is non-blank, passed tryStartTurn's emptiness + // check, and became the entire prompt of an injected turn. + const result = spawnSync(BASH_BIN, [path.join(SCRIPTS_DIR, "inbox.sh"), pair.team, pair.name, "--quiet"], { cwd: this.opts.project, encoding: "utf8" }); if (result.error || result.status !== 0) { console.error(`codex-bridge: inbox.sh failed for ${pair.team}/${pair.name}`); continue; } if ((result.stdout || "").trim()) sections.push(result.stdout.trim()); } diff --git a/tests/test_codex_bridge.bats b/tests/test_codex_bridge.bats index a87106d08..524c5ac7f 100644 --- a/tests/test_codex_bridge.bats +++ b/tests/test_codex_bridge.bats @@ -1994,3 +1994,297 @@ EOF grep -q "thread/resume" "$log" ! grep -q "process/spawn" "$log" } + +@test "codex-bridge: one wake starts one turn even when the previous turn's tail lands mid turn/start (duplicate-turn injection)" { + run node -e 'const r = require("child_process").spawnSync("/bin/sh", ["-c", "true"]); if (r.error) { console.error(r.error.message); process.exit(1); }' + if [ "$status" -ne 0 ]; then + skip "node child_process.spawn is not available in this sandbox" + fi + + # Regression for a live-observed duplicate-turn injection. A wake deferred + # behind a running turn is delivered from onTurnEnded() when turn/completed + # arrives -- and while the resulting turn/start request is still IN FLIGHT, + # the app-server's independent thread/status idle for that SAME previous + # turn lands. With the wake claim only cleared after the request resolved, + # that second turn-end re-entered tryStartTurn() with the same wake and + # started a second turn whose whole prompt was inbox.sh's literal + # "No new messages." output (the first read had already consumed the rows). + local fake="$TEST_SKILL_DIR/fake-app-server-midstart-tail.js" + local log="$TEST_SKILL_DIR/fake-app-server-midstart-tail.log" + cat >"$fake" <<'EOF' +const fs = require("fs"); +const readline = require("readline"); +const { spawnSync } = require("child_process"); +const log = process.argv[2]; +const scripts = process.argv[3]; +const rl = readline.createInterface({ input: process.stdin }); +let turns = 0; +let spawns = 0; +function send(value) { process.stdout.write(`${JSON.stringify(value)}\n`); } +rl.on("line", (line) => { + const message = JSON.parse(line); + if (message.method === "turn/start") { + const text = ((message.params.input && message.params.input[0] && message.params.input[0].text) || "").replace(/\n/g, " "); + fs.appendFileSync(log, `turn/start ${text}\n`); + } else { + fs.appendFileSync(log, `${message.method}\n`); + } + if (message.method === "initialize") { + send({ jsonrpc: "2.0", id: message.id, result: {} }); + } else if (message.method === "thread/resume") { + // Resume an ACTIVE thread (a human turn is in flight); the wake defers. + send({ jsonrpc: "2.0", id: message.id, result: { thread: { id: message.params.threadId, status: { type: "active" } } } }); + // The human turn reports completion; onTurnEnded delivers the wake. + setTimeout(() => { + send({ jsonrpc: "2.0", method: "turn/completed", params: { threadId: message.params.threadId } }); + }, 80); + } else if (message.method === "process/spawn") { + spawns += 1; + // Wake 2 exists so the run terminates via --max-wakes; give it a real + // unread row so it starts a normal (non-empty) turn. + if (spawns === 2) { + spawnSync("bash", [`${scripts}/send.sh`, "team", "bob", "alice", "wake race probe two"], { encoding: "utf8" }); + } + const id = spawns === 1 ? 5 : 6; + send({ jsonrpc: "2.0", id: message.id, result: {} }); + setTimeout(() => { + send({ jsonrpc: "2.0", method: "process/exited", params: { processHandle: message.params.processHandle, exitCode: 0, stdout: `status=pending count=1 max_id=${id}\n`, stderr: "" } }); + }, 10); + } else if (message.method === "turn/start") { + turns += 1; + if (turns === 1) { + // The previous turn's OTHER tail signal arrives while this request is + // still unanswered... + setTimeout(() => { + send({ jsonrpc: "2.0", method: "thread/status/changed", params: { threadId: message.params.threadId, status: { type: "idle" } } }); + }, 20); + // ...and only later does the request resolve; the turn then completes. + setTimeout(() => { + send({ jsonrpc: "2.0", id: message.id, result: {} }); + setTimeout(() => { + send({ jsonrpc: "2.0", method: "turn/completed", params: { threadId: message.params.threadId } }); + }, 20); + }, 120); + } else { + send({ jsonrpc: "2.0", id: message.id, result: {} }); + setTimeout(() => { + send({ jsonrpc: "2.0", method: "turn/completed", params: { threadId: message.params.threadId } }); + }, 10); + } + } else if (message.method === "process/kill") { + send({ jsonrpc: "2.0", id: message.id, result: {} }); + } +}); +EOF + + # The unread row wake 1 will deliver inline. + bash "$SCRIPTS/send.sh" team bob alice "wake race probe one" >/dev/null + + AGMSG_CODEX_APP_SERVER_CMD="node $fake $log $SCRIPTS" run node "$TYPES/codex/codex-bridge.js" \ + --project "$PROJ" --team team --name alice --thread thread-race \ + --timeout 1 --interval 1 --turn-timeout 30 --max-wakes 2 --inline-inbox + + # `grep -q` rather than `[[ ]]` in the non-last positions: on bash 3.2, + # which is what macOS CI runs, a false `[[ ]]` there reports ok. Negated + # checks are written as count comparisons for the same reason (`! cmd` + # never trips errexit). + [ "$status" -eq 0 ] + printf '%s\n' "$output" | grep -Fq "wakeup 1" + printf '%s\n' "$output" | grep -Fq "wakeup 2" + # Exactly one turn per wake: the mid-start tail must not mint a third. + [ "$(grep -c "^turn/start" "$log")" -eq 2 ] + # The duplicate wake must not be spent AT ALL — not even on an empty + # re-read that aborts. With --quiet an injected duplicate turn is invisible + # to the two checks above (the empty re-read aborts instead of becoming a + # sentinel prompt), so pin the re-read itself never happening. + [ "$(printf '%s\n' "$output" | grep -Fc "pending wake had no inbox output")" -eq 0 ] + # And no turn may ever carry the empty-inbox sentinel as its prompt. + [ "$(grep -c "No new messages." "$log")" -eq 0 ] +} + +@test "codex-bridge: a turn fully notified before its turn/start ACK still ends promptly (deferred end, no watchdog wait)" { + run node -e 'const r = require("child_process").spawnSync("/bin/sh", ["-c", "true"]); if (r.error) { console.error(r.error.message); process.exit(1); }' + if [ "$status" -ne 0 ]; then + skip "node child_process.spawn is not available in this sandbox" + fi + + # The dual of the duplicate-turn test above: a legal app-server ordering + # notifies the NEW turn's whole lifecycle -- turn/started, then + # turn/completed -- while the turn/start request is still unanswered. + # Discarding those as "stale previous-turn tails" would leave the bridge + # waiting out the idle watchdog (or hanging with --turn-timeout 0) and skip + # the maxWakes accounting. The end must be deferred and processed right + # after the ACK. + local fake="$TEST_SKILL_DIR/fake-app-server-preack-turn.js" + local log="$TEST_SKILL_DIR/fake-app-server-preack-turn.log" + cat >"$fake" <<'EOF' +const fs = require("fs"); +const readline = require("readline"); +const log = process.argv[2]; +const rl = readline.createInterface({ input: process.stdin }); +function send(value) { process.stdout.write(`${JSON.stringify(value)}\n`); } +rl.on("line", (line) => { + const message = JSON.parse(line); + fs.appendFileSync(log, `${message.method}\n`); + if (message.method === "initialize") { + send({ jsonrpc: "2.0", id: message.id, result: {} }); + } else if (message.method === "thread/resume") { + send({ jsonrpc: "2.0", id: message.id, result: { thread: { id: message.params.threadId, status: { type: "idle" } } } }); + } else if (message.method === "process/spawn") { + send({ jsonrpc: "2.0", id: message.id, result: {} }); + setTimeout(() => { + send({ jsonrpc: "2.0", method: "process/exited", params: { processHandle: message.params.processHandle, exitCode: 0, stdout: "status=pending count=1 max_id=5\n", stderr: "" } }); + }, 10); + } else if (message.method === "turn/start") { + // The turn runs to completion before the request is ACKed. + send({ jsonrpc: "2.0", method: "turn/started", params: { threadId: message.params.threadId, turn: { id: "fast-1" } } }); + send({ jsonrpc: "2.0", method: "turn/completed", params: { threadId: message.params.threadId, turn: { id: "fast-1" } } }); + setTimeout(() => { + send({ jsonrpc: "2.0", id: message.id, result: {} }); + }, 60); + } else if (message.method === "process/kill") { + send({ jsonrpc: "2.0", id: message.id, result: {} }); + } +}); +EOF + + bash "$SCRIPTS/send.sh" team bob alice "pre-ack fast turn probe" >/dev/null + local runner + runner=$(write_bridge_timeout_runner) + + AGMSG_CODEX_APP_SERVER_CMD="node $fake $log" run node "$runner" 5000 node "$TYPES/codex/codex-bridge.js" \ + --project "$PROJ" --team team --name alice --thread thread-fast \ + --timeout 1 --interval 1 --turn-timeout 30 --max-wakes 1 --inline-inbox + + # `grep -q` rather than `[[ ]]` in the non-last positions: on bash 3.2, + # which is what macOS CI runs, a false `[[ ]]` there reports ok. + [ "$status" -eq 0 ] # not 124: ended via the deferred end, not a hang + printf '%s\n' "$output" | grep -Fq "started turn" + printf '%s\n' "$output" | grep -Fq "turn completed" +} + +@test "codex-bridge: a stale idle landing after the new turn was seen starting does not end the running turn (id attribution)" { + run node -e 'const r = require("child_process").spawnSync("/bin/sh", ["-c", "true"]); if (r.error) { console.error(r.error.message); process.exit(1); }' + if [ "$status" -ne 0 ]; then + skip "node child_process.spawn is not available in this sandbox" + fi + + # The composition of the two orderings above: the previous turn's + # turn/completed delivers the wake, the NEW turn's turn/started lands before + # the ACK -- and only THEN does the previous turn's independent + # thread/status idle straggle in. Phase-based attribution ("anything after + # the new turn was observed is the new turn's end") ends the actually- + # running new turn right after the ACK: with --max-wakes it shuts the + # bridge down before the turn's real completion. Identity-based attribution + # must drop the stale idle and end only on the id-matching turn/completed. + local fake="$TEST_SKILL_DIR/fake-app-server-stale-idle.js" + local log="$TEST_SKILL_DIR/fake-app-server-stale-idle.log" + cat >"$fake" <<'EOF' +const fs = require("fs"); +const readline = require("readline"); +const { spawnSync } = require("child_process"); +const log = process.argv[2]; +const scripts = process.argv[3]; +const rl = readline.createInterface({ input: process.stdin }); +let turns = 0; +let spawns = 0; +function send(value) { process.stdout.write(`${JSON.stringify(value)}\n`); } +// Record WHEN the bridge went away (stdin EOF if it plainly exits, SIGTERM if +// its shutdown kills the app-server child), so the test can assert it +// outlived the raced turn's real completion. +function bridgeGone() { fs.appendFileSync(log, "bridge-gone\n"); process.exit(0); } +rl.on("close", bridgeGone); +process.on("SIGTERM", bridgeGone); +process.on("SIGHUP", bridgeGone); +rl.on("line", (line) => { + const message = JSON.parse(line); + fs.appendFileSync(log, `${message.method}\n`); + if (message.method === "initialize") { + send({ jsonrpc: "2.0", id: message.id, result: {} }); + } else if (message.method === "thread/resume") { + // A human turn is in flight; the wake defers behind it. + send({ jsonrpc: "2.0", id: message.id, result: { thread: { id: message.params.threadId, status: { type: "active" } } } }); + // The human turn reports completion; onTurnEnded delivers the wake. + // (--max-wakes must be 2 here: onTurnEnded checks maxWakes before it + // delivers the pending wake, so a limit of 1 would end the bridge at the + // human turn's completion without ever starting the raced turn.) + setTimeout(() => { + send({ jsonrpc: "2.0", method: "turn/completed", params: { threadId: message.params.threadId, turn: { id: "old-1" } } }); + }, 80); + } else if (message.method === "process/spawn") { + spawns += 1; + // Wake 2 terminates the run via --max-wakes; give it a real unread row. + if (spawns === 2) { + spawnSync("bash", [`${scripts}/send.sh`, "team", "bob", "alice", "stale idle probe two"], { encoding: "utf8" }); + } + const id = spawns === 1 ? 5 : 6; + send({ jsonrpc: "2.0", id: message.id, result: {} }); + setTimeout(() => { + send({ jsonrpc: "2.0", method: "process/exited", params: { processHandle: message.params.processHandle, exitCode: 0, stdout: `status=pending count=1 max_id=${id}\n`, stderr: "" } }); + }, 10); + } else if (message.method === "turn/start") { + turns += 1; + const threadId = message.params.threadId; + if (turns === 1) { + // The new turn is seen starting... + setTimeout(() => { + send({ jsonrpc: "2.0", method: "turn/started", params: { threadId, turn: { id: "new-1" } } }); + }, 10); + // ...then the OLD turn's independent idle straggles in... + setTimeout(() => { + send({ jsonrpc: "2.0", method: "thread/status/changed", params: { threadId, status: { type: "idle" } } }); + }, 20); + // ...then the request is ACKed... + setTimeout(() => { + send({ jsonrpc: "2.0", id: message.id, result: {} }); + }, 80); + // ...and the new turn's REAL completion comes much later. The delay is + // deliberately far above the bridge's subprocess latency (send.sh / + // inbox.sh are real bash+sqlite runs): a misattributed end re-arms + // detection early and starts wake 2's turn well inside this window, so + // the ordering assertion below cannot be saved by a slow machine. + setTimeout(() => { + fs.appendFileSync(log, "true-completion-sent\n"); + send({ jsonrpc: "2.0", method: "turn/completed", params: { threadId, turn: { id: "new-1" } } }); + }, 2000); + } else { + send({ jsonrpc: "2.0", id: message.id, result: {} }); + setTimeout(() => { + send({ jsonrpc: "2.0", method: "turn/completed", params: { threadId, turn: { id: `later-${turns}` } } }); + }, 10); + } + } else if (message.method === "process/kill") { + send({ jsonrpc: "2.0", id: message.id, result: {} }); + } +}); +EOF + + bash "$SCRIPTS/send.sh" team bob alice "stale idle probe one" >/dev/null + local runner + runner=$(write_bridge_timeout_runner) + + AGMSG_CODEX_APP_SERVER_CMD="node $fake $log $SCRIPTS" run node "$runner" 10000 node "$TYPES/codex/codex-bridge.js" \ + --project "$PROJ" --team team --name alice --thread thread-stale \ + --timeout 1 --interval 1 --turn-timeout 30 --max-wakes 2 --inline-inbox + + [ "$status" -eq 0 ] + [ "$(grep -c "^turn/start" "$log")" -eq 2 ] + # The raced turn must still be running until its REAL completion: neither + # wake 2's turn (a misattributed end re-arms detection early) nor the + # bridge's own exit may appear in the log before true-completion-sent. + local completion_line second_turn_line gone_line i + completion_line="$(grep -n "^true-completion-sent" "$log" | head -1 | cut -d: -f1)" + second_turn_line="$(grep -n "^turn/start" "$log" | sed -n 2p | cut -d: -f1)" + # The fake records bridge-gone on its stdin EOF, which races the runner's + # own exit by a scheduler tick -- give it a moment to land. + for i in {1..20}; do + gone_line="$(grep -n "^bridge-gone" "$log" | head -1 | cut -d: -f1)" + [ -n "$gone_line" ] && break + sleep 0.1 + done + [ -n "$completion_line" ] + [ -n "$second_turn_line" ] + [ -n "$gone_line" ] + [ "$completion_line" -lt "$second_turn_line" ] + [ "$completion_line" -lt "$gone_line" ] +} From d354ae856358f4c5e7e4b64ad6fdca2d64404953 Mon Sep 17 00:00:00 2001 From: fujibee Date: Sat, 12 Sep 2026 21:54:46 -0700 Subject: [PATCH 13/34] test(remote): drain stdin in the third driver stub with the #755 EPIPE race (#1178) (#1180) The storage-driver-env stub in tests/remote_sync_engine.test.mjs prints and exits without reading stdin, the same shape #759 fixed in two other stubs for #755: a parent write/end landing after the child has already exited hits a closed pipe, and the call is reported as a driver failure rather than the success the assertion expects. Add the same cat >/dev/null drain #759 used; a sweep of the rest of the file found no further instances of the same shape. Fixes #1178. --- tests/remote_sync_engine.test.mjs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/tests/remote_sync_engine.test.mjs b/tests/remote_sync_engine.test.mjs index 3c5c953fb..cd02b4b51 100644 --- a/tests/remote_sync_engine.test.mjs +++ b/tests/remote_sync_engine.test.mjs @@ -2682,7 +2682,12 @@ async function withDriverEnvironment(t, root, script, buildCalls) { test("storage driver subprocess cannot observe HTTP or age identity secrets", async () => { const root = await mkdtemp(join(tmpdir(), "agmsg-sync-driver-env-")); const mock = join(root, "driver.sh"); + // Same EPIPE race as the two stubs #759 fixed for #755: this one prints and + // exits without reading stdin, so a parent write/end that lands after exit + // hits a closed pipe and is reported as a driver failure. `cat` drains stdin + // first so the child stays alive until the parent has finished writing. await writeFile(mock, `#!/usr/bin/env bash +cat >/dev/null [ -z "\${AGMSG_SYNC_TOKEN:-}" ] || exit 99 [ -z "\${AGMSG_SYNC_TRUST_DIR:-}" ] || exit 95 [ -z "\${AGMSG_AGE_IDENTITY:-}" ] || exit 98 From 3ab9e31330ab4f03436a69f95b5ba4d647f95321 Mon Sep 17 00:00:00 2001 From: joelmitz <1820043398@campus.ouj.ac.jp> Date: Sun, 13 Sep 2026 14:09:27 +0900 Subject: [PATCH 14/34] fix(codex-bridge): give the identity lease a start token on Windows (#978) The codex bridge could not start on Windows at all: startToken() had two sources, /proc//stat and ps -o lstart=, and Windows satisfies neither, so writeLease() failed closed on an empty token every time and the launcher respawned the bridge into the same wall (#977). Add the missing Windows source -- Process.StartTime.Ticks read through PowerShell (powershell.exe, then pwsh; both return the same Ticks for a pid) -- in both codex-bridge.js startToken() and the launcher _start_token, and admit pwsh to the lease schema. One source, not a preference list: WMIC would let the writer and the reaper resolve different formats for the same process, so it is rejected explicitly. The Windows branch replaces the /proc branch rather than preceding it, because an MSYS /proc is keyed by its own pid space and would return an unrelated process. _agmsg_is_windows() covers MINGW*, MSYS*, CYGWIN* and CLANGARM*, matching the launcher existing MSYSTEM check, with a regression test that drives the CLANGARM64 branch. Fixes #977. --- .../types/codex/codex-bridge-launcher.sh | 49 +++++++++- scripts/drivers/types/codex/codex-bridge.js | 27 ++++++ tests/test_codex_bridge_launcher.bats | 97 +++++++++++++++++++ 3 files changed, 169 insertions(+), 4 deletions(-) diff --git a/scripts/drivers/types/codex/codex-bridge-launcher.sh b/scripts/drivers/types/codex/codex-bridge-launcher.sh index 98fb410ef..4eac7ed41 100755 --- a/scripts/drivers/types/codex/codex-bridge-launcher.sh +++ b/scripts/drivers/types/codex/codex-bridge-launcher.sh @@ -407,9 +407,44 @@ _REAP_WAIT_TICKS=50 # recycled pid is always distinguishable from the one we leased. # else -> `ps -o lstart=` (second precision). Echoes ""; # returns non-zero (indeterminable) so the caller fails closed. +# win32 -> Process.StartTime.Ticks via PowerShell -- the same single source +# codex-bridge.js startToken() writes, so both sides always agree. +# WMIC is deprecated and already absent from some Windows 11 installs; +# a per-side "WMIC, else PowerShell" order would let the two record +# differently-FORMATTED tokens for the same process. powershell.exe +# and pwsh return identical Ticks, so falling back between those two +# binaries introduces no such divergence. +# +# The Windows branch is taken INSTEAD of the /proc branch, not merely before it: +# MSYS/Cygwin do expose a working /proc, but it is keyed by the emulation layer's +# own pid space while a lease records the Windows pid codex-bridge.js sees as +# process.pid. Letting /proc win would return the start time of whatever +# unrelated MSYS process sits at that number -- the recycled-pid confusion this +# token exists to prevent, with nothing to signal it. +_agmsg_is_windows() { + case "${_AGMSG_UNAME_S:=$(uname -s 2>/dev/null || echo unknown)}" in + MINGW*|MSYS*|CYGWIN*|CLANGARM*) return 0 ;; + *) return 1 ;; + esac +} + _start_token() { - local pid="$1" s r tok + local pid="$1" s r tok bin local -a a + if _agmsg_is_windows; then + for bin in powershell.exe pwsh; do + # `tr -d '\r'` because PowerShell writes CRLF and Node's .trim() on the + # writer side strips it there, so both end up with the bare digits. + tok="$("$bin" -NoProfile -NonInteractive -Command \ + "(Get-Process -Id $pid).StartTime.Ticks" 2>/dev/null | tr -d '\r' | head -n 1)" + tok="${tok#"${tok%%[![:space:]]*}"}" + tok="${tok%"${tok##*[![:space:]]}"}" + case "$tok" in ''|*[!0-9]*) continue ;; esac + printf 'pwsh\t%s' "$tok" + return 0 + done + return 1 + fi if [ -r "/proc/$pid/stat" ]; then s="$(cat "/proc/$pid/stat" 2>/dev/null)" || return 1 r="${s##*)}" @@ -433,7 +468,7 @@ _start_token() { # Parse a lease under an EXACT v=1 schema and fail closed on anything else. Sets # lproj/lpairs/lhost/lpid/lstart/lstartsrc and returns 0 only when the file is # precisely the seven expected keys, each once, no unknown or duplicate or extra -# line, hashes 40-hex, pid numeric, startsrc proc|ps. A malformed, truncated, or +# line, hashes 40-hex, pid numeric, startsrc proc|ps|pwsh. A malformed, truncated, or # tampered lease returns non-zero, so the reaper never kills on a doubtful one. _read_lease() { local file="$1" line k v nlines=0 lv="" @@ -460,10 +495,16 @@ _read_lease() { case "$lproj" in *[!0-9a-f]*|"") return 1 ;; esac; [ "${#lproj}" -eq 40 ] || return 1 case "$lpairs" in *[!0-9a-f]*|"") return 1 ;; esac; [ "${#lpairs}" -eq 40 ] || return 1 case "$lpid" in *[!0-9]*|"") return 1 ;; esac - case "$lstartsrc" in proc|ps) ;; *) return 1 ;; esac + case "$lstartsrc" in proc|ps|pwsh) ;; *) return 1 ;; esac [ -n "$lhost" ] || return 1 [ -n "$lstart" ] || return 1 - if [ "$lstartsrc" = proc ]; then case "$lstart" in *[!0-9]*|"") return 1 ;; esac; fi + # proc's starttime ticks and .NET's StartTime.Ticks are both bare integers, so + # a lease carrying anything else under either label fails closed here. ps stays + # exempt: its lstart is a human date string whose punctuation varies by + # platform, and writer and reader only ever compare it byte for byte. + case "$lstartsrc" in + proc|pwsh) case "$lstart" in *[!0-9]*|"") return 1 ;; esac ;; + esac return 0 } diff --git a/scripts/drivers/types/codex/codex-bridge.js b/scripts/drivers/types/codex/codex-bridge.js index da4165934..82b0db42f 100755 --- a/scripts/drivers/types/codex/codex-bridge.js +++ b/scripts/drivers/types/codex/codex-bridge.js @@ -1101,7 +1101,34 @@ class CodexBridge { // and the only victim would be a same-(project,pair) bridge in the // sub-ms window before it overwrites this pid's lease, self-corrected // by the launcher respawning it. + // win32 -> Process.StartTime.Ticks, read through PowerShell. Windows has no + // /proc, and MSYS's `ps` rejects -o outright, so both POSIX sources + // yield an empty token and no lease can be published at all. + // ONE source, not a preference list: WMIC is deprecated and already + // absent from some Windows 11 installs, and letting each side choose + // between WMIC and PowerShell independently would let this writer and + // _start_token (codex-bridge-launcher.sh) record differently- + // FORMATTED tokens for the same process. powershell.exe and pwsh + // return identical Ticks, so falling back between those two binaries + // is safe: the src label names the format, not the executable. startToken() { + if (process.platform === "win32") { + for (const bin of ["powershell.exe", "pwsh"]) { + const r = spawnSync( + bin, + [ + "-NoProfile", + "-NonInteractive", + "-Command", + `(Get-Process -Id ${process.pid}).StartTime.Ticks`, + ], + { encoding: "utf8" }, + ); + const ticks = (r.status === 0 ? (r.stdout || "") : "").trim(); + if (/^\d+$/.test(ticks)) return { src: "pwsh", token: ticks }; + } + return { src: "pwsh", token: "" }; + } try { const stat = fs.readFileSync(`/proc/${process.pid}/stat`, "utf8"); const after = stat.slice(stat.lastIndexOf(")") + 1).trim().split(/\s+/); diff --git a/tests/test_codex_bridge_launcher.bats b/tests/test_codex_bridge_launcher.bats index 8f6b72ec4..ba6391c84 100644 --- a/tests/test_codex_bridge_launcher.bats +++ b/tests/test_codex_bridge_launcher.bats @@ -808,3 +808,100 @@ _fake_alice_lease() { # sets FAKE_PID once its lease file exists kill -0 "$victim" kill "$victim" "$disp" "$parent" 2>/dev/null || true; wait "$disp" 2>/dev/null || true; wait "$victim" 2>/dev/null || true } + +# --- Windows start token: the lease schema must admit the source +# codex-bridge.js writeLease() records on Windows, where /proc does not exist and +# the only `ps` likely to be on PATH (MSYS's) rejects -o outright, so both POSIX +# sources yield an empty token and the bridge can never publish a lease at all. +# +# _read_lease is the reaper's ONLY gate on a lease, so its accept/reject set is +# the contract. These exercise it directly -- the pattern test_remote.bats uses +# for _remote_endpoint_display -- rather than through the reaper: the reaper +# needs a spawnable bridge and a live pid, which is exactly what does not work on +# Git Bash (#567), and the schema question has nothing to do with either. Kept +# out of the `windows-native` filter deliberately: nothing here runs PowerShell, +# so these belong on every leg, not only the Windows one. --- +_lease_verdict() { # -> prints accept|reject + local h40=0123456789abcdef0123456789abcdef01234567 + printf 'v=1\nproject=%s\npairs=%s\nhost=h\npid=123\nstart=%s\nstartsrc=%s\n' \ + "$h40" "$h40" "$2" "$1" > "$TEST_SKILL_DIR/lease-under-test" + bash -c ' + pattern="/^_read_lease() {/,/^}/p" + eval "$(sed -n "$pattern" "$1")" + _read_lease "$2" && echo accept || echo reject + ' _ "$LAUNCHER" "$TEST_SKILL_DIR/lease-under-test" 2>/dev/null +} + +@test "launcher: the lease schema admits a pwsh start token" { + [ "$(_lease_verdict pwsh 639231441791462826)" = accept ] +} + +@test "launcher: a pwsh lease whose token is not an integer is rejected, fail-closed" { + # .NET Ticks is a bare integer. Anything else under that label is a lease this + # side did not write, and a doubtful lease must never authorise a kill. + [ "$(_lease_verdict pwsh 6392314.5)" = reject ] + [ "$(_lease_verdict pwsh '')" = reject ] +} + +@test "launcher: an unrecognised startsrc is rejected, fail-closed" { + # wmic is here on purpose, not as an arbitrary bad value: WMIC's CreationDate + # was the faster candidate and was deliberately NOT adopted, because a per-side + # "WMIC, else PowerShell" order lets the writer and the reaper resolve different + # sources for the same process whenever only one of them can reach wmic.exe. + # Rejecting the label pins that decision, so reintroducing it fails loudly. + [ "$(_lease_verdict wmic 20260824041348.411807+540)" = reject ] + [ "$(_lease_verdict bogus 123)" = reject ] +} + +@test "launcher: proc and ps leases still parse (start-token regression)" { + [ "$(_lease_verdict proc 396341883)" = accept ] + [ "$(_lease_verdict ps 'Sun Aug 24 04:00:00 2026')" = accept ] + # ps stays exempt from the integer check (its token is a human date string + # whose punctuation varies by platform); proc does not. + [ "$(_lease_verdict proc abc)" = reject ] +} + +_run_start_token() { # -> runs _start_token in a subshell + run bash -c ' + pattern="/^_agmsg_is_windows() {/,/^}/p;/^_start_token() {/,/^}/p" + eval "$(sed -n "$pattern" "$1")" + _start_token "$2" + ' _ "$LAUNCHER" "$1" +} + +@test "launcher: a live pid yields a proc or ps start token on POSIX" { + skip_on_windows "Windows has its own source; see the windows-native case" + _run_start_token $$ + [ "$status" -eq 0 ] + local tab; tab=$(printf '\t') + case "${output%%"$tab"*}" in proc|ps) ;; *) false ;; esac + [ -n "${output#*"$tab"}" ] +} + +@test "launcher: CLANGARM uname selects the Windows start token path" { + local stubdir="$TEST_SKILL_DIR/clangarm-bin" + mkdir -p "$stubdir" + printf '%s\n' '#!/usr/bin/env bash' 'printf "%s\n" CLANGARM64_NT-10.0' > "$stubdir/uname" + printf '%s\n' '#!/usr/bin/env bash' 'printf "%s\n" 639231441791462826' > "$stubdir/powershell.exe" + chmod +x "$stubdir/uname" "$stubdir/powershell.exe" + + PATH="$stubdir:$PATH" _run_start_token 123 + [ "$status" -eq 0 ] + [ "$output" = $'pwsh\t639231441791462826' ] +} + +@test "launcher: windows-native a live pid yields an integer pwsh start token" { + skip_unless_windows "PowerShell and the Windows pid space are the point" + # The pid must be the WINDOWS one. MSYS/Cygwin number processes in their own + # space -- the same shell is MSYS pid 3994449 and winpid 19568 on our runner -- + # and Get-Process only knows the latter, which is also the pid + # codex-bridge.js records as process.pid. + local winpid; winpid="$(cat /proc/$$/winpid)" + [ -n "$winpid" ] + _run_start_token "$winpid" + [ "$status" -eq 0 ] + local tab; tab=$(printf '\t') + [ "${output%%"$tab"*}" = pwsh ] + local tok="${output#*"$tab"}" + case "$tok" in ''|*[!0-9]*) false ;; esac +} From 7c1547f18719dffbb511e75a344180454b96dcce Mon Sep 17 00:00:00 2001 From: joelmitz <1820043398@campus.ouj.ac.jp> Date: Mon, 14 Sep 2026 11:30:57 +0900 Subject: [PATCH 15/34] fix(#777): the remaining argv-to-stdin sites, plus a Windows CRLF row-separator bug (#991) Part 1 (#777): inbox.sh, check-inbox.sh, watch.sh, codex watch-once.sh and storage_read_cursor_consume interpolated a growing unread backlog into one sqlite3 argv element, which fails past the per-argument ceiling and, because the batch is never marked read, never clears itself. Write each statement to a temp file and pass it on stdin, the shape history.sh already uses; the long-lived watch.sh loop and the shared storage function clean up inline instead of installing traps that would displace the watcher's own. Part 2: sqlite3.exe on Windows ends every row of a multi-row result with CRLF. Command substitution strips only the final newline, so every row but the last kept a CR on its final field -- typically an id -- and mark-as-read matched nothing (a 100-message backlog cleared one message per run). agmsg_sqlite() now strips only a CR immediately before the line end, leaving a CR inside a message body untouched, on both the direct and the recording path, with the pipeline status taken from sqlite3 so a SQLITE_BUSY still reaches the #1001 diagnostic, including under set -e callers. Refs #777. --- scripts/check-inbox.sh | 31 ++- scripts/drivers/storage/sqlite.sh | 43 +++- scripts/drivers/types/codex/watch-once.sh | 23 +- scripts/inbox.sh | 30 ++- scripts/lib/storage.sh | 107 ++++++++- scripts/watch.sh | 55 ++++- tests/test_helper.bash | 28 +++ tests/test_inbox.bats | 39 ++++ tests/test_sqlite_crlf.bats | 258 ++++++++++++++++++++++ tests/test_watch.bats | 47 ++++ tests/test_watch_once.bats | 21 ++ 11 files changed, 640 insertions(+), 42 deletions(-) create mode 100644 tests/test_sqlite_crlf.bats diff --git a/scripts/check-inbox.sh b/scripts/check-inbox.sh index b14939137..64795abe3 100755 --- a/scripts/check-inbox.sh +++ b/scripts/check-inbox.sh @@ -283,13 +283,30 @@ for team in "${TEAM_LIST[@]}"; do # _sqlite_sync_lit_into in sqlite-sync.sh, which documents the same hazard. _AGMSG_SQ="'" _arr="[$(printf '%s' "$UNREAD_JSONL" | paste -sd, -)]" - agmsg_sqlite ':memory:' " - SELECT json_extract(value,'\$.from') || char(31) || - replace(replace(json_extract(value,'\$.body'), char(10), '\n'), char(9), '\t') || char(31) || - json_extract(value,'\$.at') || char(31) || - json_extract(value,'\$.id') - FROM json_each('${_arr//$_AGMSG_SQ/$_AGMSG_SQ$_AGMSG_SQ}'); - " + # #777: this team's unread backlog grows with every message sent to it, so + # interpolating it into ONE argv element eventually exceeds the OS's + # per-argument ceiling (Linux MAX_ARG_STRLEN=131,072 bytes; smaller still + # on Windows/macOS) and `agmsg_sqlite` fails with "Argument list too + # long" -- every single poll, since the backlog that triggered it never + # shrinks on its own. Pass the statement on stdin instead, mirroring + # drivers/storage/sqlite-sync.sh:1301 (`_sqlite_data_stdin`, #882) and + # history.sh/inbox.sh: printf is a bash builtin, so writing a large value + # to a temp file never execs and can hit neither that ceiling nor argv's + # at all. The temp file is scoped to THIS subshell -- its EXIT trap fires + # when the subshell itself exits (success, `exit 98`/`exit 13` above, or a + # signal), never touching the outer script's own traps. + _agmsg_ci_sql=$(mktemp "${TMPDIR:-/tmp}/agmsg-checkinbox-rows.XXXXXX") || exit 13 + trap 'rm -f "$_agmsg_ci_sql"' EXIT HUP INT TERM + { + printf "%s\n" "SELECT json_extract(value,'\$.from') || char(31) ||" + printf "%s\n" " replace(replace(json_extract(value,'\$.body'), char(10), '\n'), char(9), '\t') || char(31) ||" + printf "%s\n" " json_extract(value,'\$.at') || char(31) ||" + printf "%s\n" " json_extract(value,'\$.id')" + printf "FROM json_each('" + printf '%s' "${_arr//$_AGMSG_SQ/$_AGMSG_SQ$_AGMSG_SQ}" + printf "');\n" + } > "$_agmsg_ci_sql" + agmsg_sqlite ':memory:' < "$_agmsg_ci_sql" ) _rc=$? set -e diff --git a/scripts/drivers/storage/sqlite.sh b/scripts/drivers/storage/sqlite.sh index 47bda31e1..763ee0083 100755 --- a/scripts/drivers/storage/sqlite.sh +++ b/scripts/drivers/storage/sqlite.sh @@ -378,9 +378,36 @@ storage_read_cursor_consume() { WHERE e.type='message_sent' AND e.team='$tl' AND e.id='$(_sqlite_lit "$id")' AND e.legacy_id IS NOT NULL);" done - agmsg_sqlite "$db" "BEGIN IMMEDIATE; - $sql - INSERT OR IGNORE INTO read_cursors(team,agent,local_position) + # #777 ("Not measured" section): $sql gains one INSERT/UPDATE block per + # delivered id, and the whole "BEGIN IMMEDIATE; ...; COMMIT;" statement + # used to be handed to `agmsg_sqlite` as ONE argv element. Measured on + # Windows: 97 ids built a 38,897-byte statement and CreateProcess refused + # it outright (that ceiling is 32,767 characters -- well under Linux's own + # MAX_ARG_STRLEN=131,072 bytes) -- and the failure was masked further, + # surfacing only as this function's ordinary runtime_error/13 return, never + # as a visible "Argument list too long". Same fix as history.sh / + # inbox.sh / check-inbox.sh / watch.sh / watch-once.sh, and + # drivers/storage/sqlite-sync.sh's own #882 fix: write the statement to a + # temp file with printf (a bash builtin, so it never execs) and feed + # `agmsg_sqlite` the statement on stdin instead. + # + # No trap here, on purpose: this is a SHARED LIBRARY FUNCTION, called every + # poll from watch.sh's long-lived loop, which installs its own permanent + # `trap cleanup EXIT` / `trap 'exit 0' INT TERM HUP` once near the top of + # that process. A trap set and then cleared in here (bash traps do not + # stack) would replace watch.sh's for the rest of its life the first time + # this function ever ran -- the exact mistake this same #777 pass caught + # and avoided in watch.sh's own ROWS-fetch fix a few lines above this one + # in the call chain. The temp file is removed explicitly on every path + # instead; the one path that leaks it (a signal landing mid-call) is left + # for the OS's own temp-directory cleanup, same trade-off already accepted + # there. + local sql_file + sql_file=$(mktemp "${TMPDIR:-/tmp}/agmsg-cursor-consume.XXXXXX" 2>/dev/null) || { echo runtime_error; return 13; } + { + printf '%s\n' "BEGIN IMMEDIATE;" + printf '%s\n' "$sql" + printf '%s\n' " INSERT OR IGNORE INTO read_cursors(team,agent,local_position) VALUES('$tl','$al',0); UPDATE read_cursors SET local_position=MAX(local_position,COALESCE(( SELECT MIN(e.seq)-1 FROM events e @@ -390,8 +417,14 @@ storage_read_cursor_consume() { AND NOT EXISTS(SELECT 1 FROM events r WHERE r.type='message_read' AND r.team=e.team AND r.agent='$al' AND r.msg_id=e.id) ),MIN($target,$(_sqlite_highwater)))) - WHERE team='$tl' AND agent='$al'; - COMMIT;" >/dev/null 2>&1 || { echo runtime_error; return 13; } + WHERE team='$tl' AND agent='$al';" + printf '%s\n' "COMMIT;" + } > "$sql_file" + if ! agmsg_sqlite "$db" < "$sql_file" >/dev/null 2>&1; then + rm -f "$sql_file" + echo runtime_error; return 13 + fi + rm -f "$sql_file" echo ok } diff --git a/scripts/drivers/types/codex/watch-once.sh b/scripts/drivers/types/codex/watch-once.sh index 415c2b86b..1c23f9d06 100755 --- a/scripts/drivers/types/codex/watch-once.sh +++ b/scripts/drivers/types/codex/watch-once.sh @@ -127,9 +127,26 @@ while true; do u="$(storage_list_unread "$_team" "$_agent" 2>/dev/null || true)" [ -n "$u" ] || continue uarr="[$(printf '%s' "$u" | paste -sd, -)]" - ids="$(agmsg_sqlite ':memory:' " - SELECT json_extract(value,'\$.id') FROM json_each('$(printf '%s' "$uarr" | sed "s/'/''/g")'); - " 2>/dev/null || true)" + # #777: this pair's unread backlog grows with every message sent to it, + # so interpolating it into ONE argv element eventually exceeds the OS's + # per-argument ceiling (Linux MAX_ARG_STRLEN=131,072 bytes; smaller + # still on Windows/macOS) and `agmsg_sqlite` fails with "Argument list + # too long" -- every single poll, since the backlog that triggered it + # never shrinks on its own (this script never marks anything read; see + # the file header). Pass the statement on stdin instead, mirroring + # drivers/storage/sqlite-sync.sh:1301 (`_sqlite_data_stdin`, #882) and + # history.sh/inbox.sh: printf is a bash builtin, so writing a large + # value to a temp file never execs and can hit neither that ceiling nor + # argv's at all. `|| continue` on mktemp failure matches the existing + # per-pair `continue` a few lines above: one pair's storage error must + # not end the whole subscription's poll. + _agmsg_wo_sql=$(mktemp "${TMPDIR:-/tmp}/agmsg-watchonce-ids.XXXXXX" 2>/dev/null) || continue + trap 'rm -f "$_agmsg_wo_sql"' EXIT HUP INT TERM + printf "%s\n" "SELECT json_extract(value,'\$.id') FROM json_each('$(printf '%s' "$uarr" | sed "s/'/''/g")');" \ + > "$_agmsg_wo_sql" + ids="$(agmsg_sqlite ':memory:' < "$_agmsg_wo_sql" 2>/dev/null || true)" + rm -f "$_agmsg_wo_sql" + trap - EXIT HUP INT TERM [ -n "$ids" ] || continue count=$(( count + $(printf '%s\n' "$ids" | grep -c .) )) all_ids="$all_ids$ids"$'\n' diff --git a/scripts/inbox.sh b/scripts/inbox.sh index c44a12259..8a887bcff 100755 --- a/scripts/inbox.sh +++ b/scripts/inbox.sh @@ -48,13 +48,29 @@ fi # _sqlite_sync_lit_into in sqlite-sync.sh, which documents the same hazard. _AGMSG_SQ="'" _arr="[$(printf '%s' "$UNREAD_JSONL" | paste -sd, -)]" -ROWS=$(agmsg_sqlite ':memory:' " - SELECT json_extract(value,'\$.from') || char(31) || - replace(replace(json_extract(value,'\$.body'), char(10), '\n'), char(9), '\t') || char(31) || - json_extract(value,'\$.at') || char(31) || - json_extract(value,'\$.id') - FROM json_each('${_arr//$_AGMSG_SQ/$_AGMSG_SQ$_AGMSG_SQ}'); -") +# #777: an agent's unread backlog grows with every message sent to it, so +# interpolating it into ONE argv element eventually exceeds the OS's +# per-argument ceiling (Linux MAX_ARG_STRLEN=131,072 bytes; smaller still on +# Windows/macOS) and `agmsg_sqlite` fails with "Argument list too long" -- +# every single call, since the backlog that triggered it never shrinks on +# its own. Pass the statement on stdin instead, mirroring +# drivers/storage/sqlite-sync.sh:1301 (`_sqlite_data_stdin`, #882) and +# history.sh: printf is a bash builtin, so writing a large value to a temp +# file never execs and can hit neither that ceiling nor argv's at all. +_agmsg_inbox_sql=$(mktemp "${TMPDIR:-/tmp}/agmsg-inbox-rows.XXXXXX") || exit 13 +trap 'rm -f "$_agmsg_inbox_sql"' EXIT HUP INT TERM +{ + printf "%s\n" "SELECT json_extract(value,'\$.from') || char(31) ||" + printf "%s\n" " replace(replace(json_extract(value,'\$.body'), char(10), '\n'), char(9), '\t') || char(31) ||" + printf "%s\n" " json_extract(value,'\$.at') || char(31) ||" + printf "%s\n" " json_extract(value,'\$.id')" + printf "FROM json_each('" + printf '%s' "${_arr//$_AGMSG_SQ/$_AGMSG_SQ$_AGMSG_SQ}" + printf "');\n" +} > "$_agmsg_inbox_sql" +ROWS=$(agmsg_sqlite ':memory:' < "$_agmsg_inbox_sql") +rm -f "$_agmsg_inbox_sql" +trap - EXIT HUP INT TERM COUNT=$(printf '%s\n' "$ROWS" | wc -l | tr -d ' ') echo "$COUNT new message(s):" diff --git a/scripts/lib/storage.sh b/scripts/lib/storage.sh index 6283f22ed..1c5e35b65 100644 --- a/scripts/lib/storage.sh +++ b/scripts/lib/storage.sh @@ -253,9 +253,48 @@ agmsg_sqlite() { _agmsg_sqlite_recording "$@" return fi - # shellcheck disable=SC2086 # intentional split: "-escape off" → two args, or none + # Windows' sqlite3.exe (measured: 3.53.4) ends each row of a multi-row + # result with \r\n, not \n -- confirmed by piping a three-row SELECT + # through `od -c` on real Windows hardware. This is independent of the + # `-escape` probe above (#102/#143: that is sqlite3 >= 3.50's own caret- + # notation rendering, fixed by `-escape off`, and reproduces on Linux too + # -- this CRLF ending does not reproduce here). HYPOTHESIS (unverified): + # the Windows C runtime's stdio text-mode translation rewrites sqlite3's + # own LF terminators to CRLF on the way out; what is actually confirmed is + # only the \r\n on the wire, not this mechanism. + # + # `ROWS=$(agmsg_sqlite ...)` strips only the trailing newline of the WHOLE + # captured output (bash command substitution), so every row but the last + # keeps a \r stuck to its final field -- typically an id, since every + # multi-field row built by this codebase's callers puts id/cursor/at last + # and body earlier (never in scope for this fix, but worth naming: it is + # why this hazard has not already shown up as corrupted message bodies). + # `IFS=$'\x1f' read` does not split on \r, so that \r rides along into + # the field value. Reported and measured on real Windows hardware: a + # 100-message backlog lost 99 of 100 mark-as-read updates in one + # inbox.sh run, because storage_mark_read_batch's ids no longer matched + # any real msg_id. + # + # The fix normalizes ONLY a \r immediately before the line-ending \n -- + # not every \r in the stream. `tr -d '\r'` (used by _sqlite_data / + # _sqlite_data_stdin in drivers/storage/sqlite.sh, wrapping calls to THIS + # function) would also be correct for THIS symptom, but it deletes every + # \r anywhere in the output, including one that is a message body's own + # content (char(13) is not replaced the way char(10) already is in every + # row-building SELECT in this codebase) -- so it is not used here. `sed`'s + # `$` anchor matches only end-of-line, so a \r elsewhere in a row + # (mid-body) is left untouched. + # + # Wrapped in a subshell with its own `set -o pipefail` so the pipeline's + # status is sqlite3's, not sed's, without changing pipefail for the + # calling script (same shape as _sqlite_data / _sqlite_data_stdin in + # drivers/storage/sqlite.sh). local _agmsg_sqlite_rc=0 - sqlite3 $_AGMSG_ESCAPE_FLAG -cmd ".timeout ${AGMSG_BUSY_TIMEOUT:-5000}" "$@" || _agmsg_sqlite_rc=$? + ( + set -o pipefail + # shellcheck disable=SC2086 # intentional split: "-escape off" → two args, or none + sqlite3 $_AGMSG_ESCAPE_FLAG -cmd ".timeout ${AGMSG_BUSY_TIMEOUT:-5000}" "$@" | sed $'s/\r$//' + ) || _agmsg_sqlite_rc=$? # SQLITE_BUSY after the full timeout used to pass in silence: the caller saw # a non-zero it often swallowed, and the operator saw a command that hung # for the timeout and said nothing (#1001 -- two people diagnosed two @@ -285,16 +324,66 @@ agmsg_sqlite() { # failed, so "the operation failed and the last statement was busy" names it. # # stderr is captured to classify it and re-emitted unchanged, so a caller that -# reads or silences it sees what it saw before; stdout is the data stream and -# is not touched; the exit status is passed through. Written as an `if` so a -# caller running under `set -e` is not exited by the assignment itself. +# reads or silences it sees what it saw before; stdout is the data stream, now +# passed through the same trailing-CR normalization as agmsg_sqlite()'s own +# non-recording path above (Windows' sqlite3.exe row-separator \r\n; see that +# comment for the full writeup -- this path bypasses it entirely via the early +# `return` above, so it needs its own copy of the fix, not a call into it: this +# function's stdout/stderr routing exists for a different purpose, classifying +# ok/busy/failed for the sync driver adapter, and folding the two together +# would tangle two independent concerns). The exit status is still passed +# through, unaffected either way. +# +# The original fd-3 passthrough trick (sqlite3's own fd 1 repointed at +# whatever fd 1 was outside this function, with no process in between) cannot +# survive inserting `sed`: stdout now goes through an actual pipe, so a temp +# file replaces the `err=$(...)` capture for stderr, and the exit status comes +# from `${PIPESTATUS[0]}` (sqlite3's, not sed's) rather than the substitution's +# own `$?`. Stderr is still read back whole and re-emitted verbatim afterward, +# so a caller that reads or silences it sees the same bytes as before. +# +# The pipeline is wrapped in an `if`, same as the original, and for the same +# reason: this is a plain function call, not a subshell, so it runs in the +# CALLING script's own shell -- and several callers set both `-e` and +# `-o pipefail`. A command tested by `if` is exempt from `set -e` on a +# non-zero exit (POSIX), so the pipeline cannot abort the caller here +# regardless of its pipefail setting. +# +# `${PIPESTATUS[0]}` (sqlite3's exit status, not sed's) is read in BOTH +# branches, not once after the `if` -- and specifically not guarded with +# `|| true` the way the CRLF fix above is, because `|| true` is not safe +# here. `PIPESTATUS` is overwritten by the NEXT command this shell +# executes, of any kind, including a trivial one: `pipeline || true` runs +# `true` whenever the pipeline's own exit status is non-zero, and reading +# `${PIPESTATUS[0]}` after that reads back `true`'s status (0), not +# sqlite3's. The CRLF fix's own `|| true` above is fine BECAUSE that call +# site never reads PIPESTATUS at all. This one silently turned every +# failure here into rc=0 whenever pipefail was already active in the +# caller -- and only there: storage-sync-driver.sh sets `-o pipefail` +# itself, so a plain `bash -c` probe without it stayed green while the +# real busy-timeout contract test (test_remote_sync.bats, "a store +# another writer holds is busy") got 0 where it expected 11. Reading +# PIPESTATUS inside the `if`'s own branches, before anything else runs, +# is what keeps it correct either way. _agmsg_sqlite_recording() { - local err rc + local err rc errfile + # A mktemp failure degrades stderr capture to /dev/null rather than failing + # the operation outright: worse diagnostics (an unclassifiable error reads + # as "failed", never as "busy"), not worse correctness, and the same + # "environment problem, not a bad input" class of failure the busy/failed + # distinction exists to tell apart from an ordinary refusal. + errfile=$(mktemp "${TMPDIR:-/tmp}/agmsg-sqlite-recording-err.XXXXXX" 2>/dev/null) || errfile=/dev/null # shellcheck disable=SC2086 # same intentional split as above - if { err=$(sqlite3 $_AGMSG_ESCAPE_FLAG -cmd ".timeout ${AGMSG_BUSY_TIMEOUT:-5000}" "$@" 2>&1 >&3 3>&-); } 3>&1; then - rc=0 + if sqlite3 $_AGMSG_ESCAPE_FLAG -cmd ".timeout ${AGMSG_BUSY_TIMEOUT:-5000}" "$@" 2>"$errfile" | sed $'s/\r$//'; then + rc=${PIPESTATUS[0]} + else + rc=${PIPESTATUS[0]} + fi + if [ "$errfile" = /dev/null ]; then + err="" else - rc=$? + err="$(cat "$errfile" 2>/dev/null)" + rm -f "$errfile" fi [ -z "$err" ] || printf '%s\n' "$err" >&2 if [ "$rc" -eq 0 ]; then diff --git a/scripts/watch.sh b/scripts/watch.sh index a3c59f484..00d633e15 100755 --- a/scripts/watch.sh +++ b/scripts/watch.sh @@ -698,17 +698,50 @@ while true; do # _sqlite_sync_lit_into in sqlite-sync.sh, which documents the same hazard. _AGMSG_SQ="'" _arr="[$(printf '%s' "$OUT" | paste -sd, -)]" - ROWS="$(agmsg_sqlite ':memory:' " - SELECT COALESCE(json_extract(value,'\$.type'),'') || char(31) || - COALESCE(json_extract(value,'\$.id'),'') || char(31) || - COALESCE(json_extract(value,'\$.at'),'') || char(31) || - COALESCE(json_extract(value,'\$.team'),'') || char(31) || - COALESCE(json_extract(value,'\$.from'),'') || char(31) || - COALESCE(json_extract(value,'\$.to'),'') || char(31) || - replace(replace(replace(COALESCE(json_extract(value,'\$.body'),''), char(13), ''), char(10), '\\n'), char(9), '\t') || char(31) || - COALESCE(json_extract(value,'\$.cursor'),'') - FROM json_each('${_arr//$_AGMSG_SQ/$_AGMSG_SQ$_AGMSG_SQ}'); - " 2>/dev/null || true)" + # #777: this pair's undelivered backlog grows independently of anything + # this loop bounds, so interpolating it into ONE argv element eventually + # exceeds the OS's per-argument ceiling (Linux MAX_ARG_STRLEN=131,072 + # bytes; smaller still on Windows/macOS) and `agmsg_sqlite` fails with + # "Argument list too long" -- every single poll, because the failure + # below was already swallowed by `|| true` and the read cursor is only + # advanced from FINAL_CURSOR/DELIVERED_IDS further down, so a silently + # empty ROWS here left the cursor stuck forever, repeating the same + # failure on every future poll. Pass the statement on stdin instead, + # mirroring drivers/storage/sqlite-sync.sh:1301 (`_sqlite_data_stdin`, + # #882) and history.sh/inbox.sh: printf is a bash builtin, so writing a + # large value to a temp file never execs and can hit neither that ceiling + # nor argv's at all. + # + # No trap here: this script installs `trap cleanup EXIT` and + # `trap 'exit 0' INT TERM HUP` once, near the top (bash traps do not + # stack -- the last one set wins), and this runs inside that same + # process's long-lived polling loop, once per pair per interval. Adding a + # loop-local trap here would silently replace those for the rest of the + # process's life. The temp file is removed explicitly on every path + # instead; the one path that leaks it (a signal landing between mktemp + # and the following rm) is caught by the pre-existing INT/TERM/HUP + # handler tearing down the whole process, same as any other in-flight + # work here. + _agmsg_watch_sql="$(mktemp "${TMPDIR:-/tmp}/agmsg-watch-rows.XXXXXX" 2>/dev/null || true)" + if [ -n "$_agmsg_watch_sql" ]; then + { + printf "%s\n" "SELECT COALESCE(json_extract(value,'\$.type'),'') || char(31) ||" + printf "%s\n" " COALESCE(json_extract(value,'\$.id'),'') || char(31) ||" + printf "%s\n" " COALESCE(json_extract(value,'\$.at'),'') || char(31) ||" + printf "%s\n" " COALESCE(json_extract(value,'\$.team'),'') || char(31) ||" + printf "%s\n" " COALESCE(json_extract(value,'\$.from'),'') || char(31) ||" + printf "%s\n" " COALESCE(json_extract(value,'\$.to'),'') || char(31) ||" + printf "%s\n" " replace(replace(replace(COALESCE(json_extract(value,'\$.body'),''), char(13), ''), char(10), '\\n'), char(9), '\t') || char(31) ||" + printf "%s\n" " COALESCE(json_extract(value,'\$.cursor'),'')" + printf "FROM json_each('" + printf '%s' "${_arr//$_AGMSG_SQ/$_AGMSG_SQ$_AGMSG_SQ}" + printf "');\n" + } > "$_agmsg_watch_sql" + ROWS="$(agmsg_sqlite ':memory:' < "$_agmsg_watch_sql" 2>/dev/null || true)" + rm -f "$_agmsg_watch_sql" + else + ROWS="" + fi FINAL_CURSOR="" DELIVERED_IDS=() diff --git a/tests/test_helper.bash b/tests/test_helper.bash index a9c481ab6..38916b858 100644 --- a/tests/test_helper.bash +++ b/tests/test_helper.bash @@ -405,3 +405,31 @@ spawn_decoy_with_cmdline() { bash "$decoy" "$path" 3>&- & DECOY_PID=$! } + +# Sends messages of ~ bytes each from to on +# , via storage_send directly rather than send.sh's own CLI (#777 +# argv-length regressions in inbox.sh/check-inbox.sh/watch.sh/watch-once.sh). +# +# A plain bash FUNCTION CALL, not a subprocess: `storage_send "$team" ... +# "$body"` hands the body to sqlite3 through the same escaped-argv path +# production code uses for a single INSERT (which is not itself in scope -- +# no test here builds a body anywhere near that ceiling), but building the +# backlog this way never has to exec anything with the WHOLE backlog as one +# argument, which is exactly the shape production code used to get wrong +# on read. Bodies are tagged "$label-$i-" so a caller can assert both +# ends of the run (index 0 and count-1) are actually present in what the +# script under test displayed, not just that its exit status was 0. +bulk_send_direct() { + local team="$1" from="$2" to="$3" count="$4" bodylen="$5" label="$6" \ + i=0 pad + pad="$(head -c "$bodylen" /dev/zero | tr '\0' 'x')" + ( + # shellcheck disable=SC1090 + source "$SCRIPTS/lib/storage.sh" + agmsg_storage_load + while [ "$i" -lt "$count" ]; do + storage_send "$team" "$from" "$to" "${label}-${i}-${pad}" >/dev/null + i=$((i + 1)) + done + ) +} diff --git a/tests/test_inbox.bats b/tests/test_inbox.bats index ede115c6d..ecf3f7d22 100644 --- a/tests/test_inbox.bats +++ b/tests/test_inbox.bats @@ -409,3 +409,42 @@ _codex_proj() { grep -q 'additive' <<<"$output" [ "$(pair_unread_count ctm alice)" -eq 0 ] } + +# --- argv-length regression (#777) --------------------------------------- +# +# Both inbox.sh and check-inbox.sh used to embed the whole unread backlog into +# ONE argv element for `sqlite3 ':memory:' ""`. 100 messages of +# ~2000 bytes each is about 200,000 bytes of body alone, well past Linux's +# MAX_ARG_STRLEN (131,072 bytes -- measured directly in this same suite's +# environment, and documented in scripts/history.sh; the ceiling is smaller +# still on Windows: 32,767 characters). Before the fix this failed every +# single run with "Argument list too long" -- the backlog that triggered it +# never shrinks on its own, so it never recovered. + +@test "inbox: a backlog large enough to exceed the OS argv ceiling still displays and marks read (#777)" { + bulk_send_direct testteam bob alice 100 2000 BIG + + run bash "$SCRIPTS/inbox.sh" testteam alice + [ "$status" -eq 0 ] + grep -qF -- "100 new message(s):" <<< "$output" + grep -qF -- "BIG-0-" <<< "$output" + grep -qF -- "BIG-99-" <<< "$output" + [ "$(unread_count alice)" -eq 0 ] +} + +@test "check-inbox: a backlog large enough to exceed the OS argv ceiling still delivers and marks read (#777)" { + bulk_send_direct testteam bob alice 100 2000 CIBIG + + # Not delivered_to_operator() here: that helper embeds the WHOLE payload + # into its own single-shot json_valid('$esc') probe (an sqlite3 argv + # element again, just on the test side), so a body this size would trip + # the identical #777 ceiling one layer up and fail for a reason that has + # nothing to do with check-inbox.sh. Reading raw stdout directly, the way + # "multiple identities poll only the first agent's exact team rows" above + # already does, keeps this test pinned on the script under test. + run bash -c "echo '{}' | bash '$SCRIPTS/check-inbox.sh' claude-code /tmp/project-a" + [ "$status" -eq 0 ] + grep -qF -- "CIBIG-0-" <<< "$output" + grep -qF -- "CIBIG-99-" <<< "$output" + [ "$(unread_count alice)" -eq 0 ] +} diff --git a/tests/test_sqlite_crlf.bats b/tests/test_sqlite_crlf.bats new file mode 100644 index 000000000..ca3ec9c39 --- /dev/null +++ b/tests/test_sqlite_crlf.bats @@ -0,0 +1,258 @@ +#!/usr/bin/env bats + +# Regression coverage for a Windows-only sqlite3.exe behavior that this +# machine (Linux) cannot reproduce on its own: multi-row SELECT output +# separates rows with \r\n, not \n. Measured on real Windows hardware +# (sqlite3.exe 3.53.4) and reported against inbox.sh's mark-as-read step -- +# see scripts/lib/storage.sh's agmsg_sqlite() for the full writeup. This +# file exercises the fix with a PATH-shimmed sqlite3 stub that reproduces +# the \r\n row separator deterministically on Linux, the same "wrapper +# script ahead of the real binary on PATH" technique test_watch_once.bats +# already uses for its slow-awk shim. +# +# No issue number yet -- this was found independently of #777 while +# verifying #777's own fix on Windows hardware, and has not been filed +# upstream. + +load test_helper + +setup() { + setup_test_env +} + +teardown() { + teardown_test_env +} + +# A wrapper named `sqlite3`, placed ahead of the real one on PATH, that +# behaves exactly like the real binary except every line of its stdout +# gets a synthetic \r appended right before the \n -- simulating the \r\n +# Windows' sqlite3.exe emits at each row boundary. +# +# The real binary's path is resolved HERE, before this directory is ever +# prepended to PATH, and baked into the wrapper as a literal exec target. +# Mirrors test_watch_once.bats's `_slow_startup_path` awk shim exactly for +# this reason: a lookup done INSIDE the wrapper, after PATH already +# includes this directory, would resolve back to the wrapper itself. +_stub_sqlite3_crlf() { + local dir="$BATS_TEST_TMPDIR/crlfbin" real + real="$(command -v sqlite3)" + mkdir -p "$dir" + cat > "$dir/sqlite3" </dev/null + bash "$SCRIPTS/join.sh" crlfteam bob claude-code /tmp/project-crlf >/dev/null + local n + for n in $(seq 1 20); do + bash "$SCRIPTS/send.sh" crlfteam bob alice "CRLF-$n" >/dev/null + done + + PATH="$stub:$PATH" + run bash "$SCRIPTS/inbox.sh" crlfteam alice + [ "$status" -eq 0 ] + grep -qF -- "20 new message(s):" <<< "$output" + for n in $(seq 1 20); do + [[ "$output" == *"CRLF-$n"* ]] + done + + # The real symptom: with the pre-fix agmsg_sqlite, only the LAST id in a + # multi-row unread scan kept a clean (unmangled) trailing field, so all + # but one mark-as-read update silently matched no real msg_id and every + # other message stayed unread. Fixed, the whole backlog clears. + local left + # `grep -c .` exits 1 when the count is 0 (no matching lines) -- correct + # and expected here, but bats runs test bodies under `set -e`, so without + # `|| true` that exit status would abort the test right at this + # assignment before the assertion below ever ran. + left="$(bash -c ' + source "'"$SCRIPTS"'/lib/storage.sh" + agmsg_storage_load + storage_list_unread crlfteam alice + ' | grep -c . || true)" + [ "$left" -eq 0 ] +} + +# --- AGMSG_SQLITE_OUTCOME_FILE (recording) path ------------------------- +# +# agmsg_sqlite() takes a completely different branch when +# AGMSG_SQLITE_OUTCOME_FILE is set (_agmsg_sqlite_recording -- only the +# sync driver adapter sets this, scripts/internal/storage-sync-driver.sh), +# so the fix above does not automatically cover it: it needs, and got, its +# own copy of the same trailing-CR normalization. A codex review of the +# first version of this fix caught the gap. Reproduces via the same +# _stub_sqlite3_crlf, plus a second, fully synthetic stub +# (_stub_sqlite3_fixed) for deterministically exercising the ok/busy/failed +# classification without depending on real SQLITE_BUSY lock-contention +# timing. + +@test "agmsg_sqlite (recording path): strips only the trailing CR, same as the non-recording path" { + local stub outfile + stub="$(_stub_sqlite3_crlf)" + outfile="$BATS_TEST_TMPDIR/outcome" + PATH="$stub:$PATH" + run bash -c "source '$SCRIPTS/lib/storage.sh'; AGMSG_SQLITE_OUTCOME_FILE='$outfile' agmsg_sqlite ':memory:' 'SELECT 1; SELECT 2; SELECT 3;'" + [ "$status" -eq 0 ] + [ "$output" = $'1\n2\n3' ] + [ "$(cat "$outfile")" = ok ] +} + +@test "agmsg_sqlite (recording path): preserves a genuine mid-body CR under the same CRLF stub" { + local stub outfile + stub="$(_stub_sqlite3_crlf)" + outfile="$BATS_TEST_TMPDIR/outcome" + PATH="$stub:$PATH" + run bash -c "source '$SCRIPTS/lib/storage.sh'; AGMSG_SQLITE_OUTCOME_FILE='$outfile' agmsg_sqlite ':memory:' \"SELECT 'x' || char(31) || 'a' || char(13) || 'b' || char(31) || 'id123';\"" + [ "$status" -eq 0 ] + [ "$output" = $'x\x1fa\rb\x1fid123' ] + [ "$(cat "$outfile")" = ok ] +} + +# A stub that ignores the real database entirely and just emits FIXED +# stdout/stderr/exit-code content read back from two plain files -- for +# testing _agmsg_sqlite_recording's own ok/busy/failed classification and +# exit-status passthrough in isolation from any real SQL execution or lock +# timing. The desired bytes are written to files by the CALLER (ordinary +# $'...' quoting there, no heredoc-embedding hazards) rather than baked into +# the generated script's own text. +_stub_sqlite3_fixed() { + local dir="$BATS_TEST_TMPDIR/fixedbin" stdout_file="$1" stderr_file="$2" exitcode="$3" + mkdir -p "$dir" + cat > "$dir/sqlite3" <&2 +exit $exitcode +EOF + chmod +x "$dir/sqlite3" + printf '%s' "$dir" +} + +@test "agmsg_sqlite (recording path): ok classification, outcome file, and exit code are unchanged" { + local stub outfile stdout_file stderr_file + stdout_file="$BATS_TEST_TMPDIR/out.txt"; stderr_file="$BATS_TEST_TMPDIR/err.txt" + printf 'row1\nrow2\n' > "$stdout_file" + printf '' > "$stderr_file" + stub="$(_stub_sqlite3_fixed "$stdout_file" "$stderr_file" 0)" + outfile="$BATS_TEST_TMPDIR/outcome" + PATH="$stub:$PATH" + run bash -c "source '$SCRIPTS/lib/storage.sh'; AGMSG_SQLITE_OUTCOME_FILE='$outfile' agmsg_sqlite ':memory:' 'irrelevant, the stub ignores it;'" + [ "$status" -eq 0 ] + [ "$output" = $'row1\nrow2' ] + [ "$(cat "$outfile")" = ok ] +} + +@test "agmsg_sqlite (recording path): busy classification, outcome file, exit code, and verbatim stderr are unchanged" { + local stub outfile stdout_file stderr_file + stdout_file="$BATS_TEST_TMPDIR/out.txt"; stderr_file="$BATS_TEST_TMPDIR/err.txt" + printf '' > "$stdout_file" + printf 'Error: database is locked\n' > "$stderr_file" + stub="$(_stub_sqlite3_fixed "$stdout_file" "$stderr_file" 5)" + outfile="$BATS_TEST_TMPDIR/outcome" + PATH="$stub:$PATH" + # --separate-stderr (bats-core, same idiom test_remote_sync.bats already + # uses) so stdout and stderr can be asserted apart -- the sed fix touches + # stdout only, so the classification text must arrive on $stderr + # byte-for-byte, not just be classified correctly. + run --separate-stderr bash -c "source '$SCRIPTS/lib/storage.sh'; AGMSG_SQLITE_OUTCOME_FILE='$outfile' agmsg_sqlite ':memory:' 'irrelevant, the stub ignores it;'" + [ "$status" -eq 5 ] + [ "$(cat "$outfile")" = busy ] + [[ "$stderr" == *"Error: database is locked"* ]] +} + +@test "agmsg_sqlite (recording path): failed classification, outcome file, exit code, and verbatim stderr are unchanged" { + local stub outfile stdout_file stderr_file + stdout_file="$BATS_TEST_TMPDIR/out.txt"; stderr_file="$BATS_TEST_TMPDIR/err.txt" + printf '' > "$stdout_file" + printf 'Error: near "not": syntax error\n' > "$stderr_file" + stub="$(_stub_sqlite3_fixed "$stdout_file" "$stderr_file" 1)" + outfile="$BATS_TEST_TMPDIR/outcome" + PATH="$stub:$PATH" + run --separate-stderr bash -c "source '$SCRIPTS/lib/storage.sh'; AGMSG_SQLITE_OUTCOME_FILE='$outfile' agmsg_sqlite ':memory:' 'irrelevant, the stub ignores it;'" + [ "$status" -eq 1 ] + [ "$(cat "$outfile")" = failed ] + [[ "$stderr" == *"syntax error"* ]] +} + +@test "agmsg_sqlite (recording path): busy classification survives a caller with -e/pipefail already on (storage-sync-driver.sh's own setting)" { + # _agmsg_sqlite_recording is a plain function call, not a subshell -- it + # runs IN the calling script's own shell, inheriting whatever `set -e` / + # `set -o pipefail` that shell already has. storage-sync-driver.sh, the + # ONLY real caller that ever sets AGMSG_SQLITE_OUTCOME_FILE, has + # `set -euo pipefail` at its own top, so this is the actual condition in + # production, not a hypothetical. + # + # A prior version of this fix guarded the pipeline with `pipeline || + # true` to keep a pipefail-inheriting caller's `set -e` from aborting + # right there. That guard is safe on its own, but this call site ALSO + # reads `${PIPESTATUS[0]}` afterward -- and PIPESTATUS is overwritten by + # the very next command this shell runs, of any kind. With pipefail on, + # the pipeline's own exit status became sqlite3's non-zero one, which is + # exactly when `|| true` runs `true` -- so `${PIPESTATUS[0]}` was read + # back as `true`'s (0), not sqlite3's, and every busy/failed call quietly + # became "ok". Only a caller with pipefail already on triggers `|| true` + # in the first place, which is why the tests above -- run from a plain + # `bash -c` with no pipefail -- stayed green through this: they never + # replicated the one caller shape that actually breaks it. Caught for + # real by test_remote_sync.bats's busy-timeout contract test, which does + # go through the real adapter and hence its `-o pipefail`. + local stub outfile stdout_file stderr_file + stdout_file="$BATS_TEST_TMPDIR/out.txt"; stderr_file="$BATS_TEST_TMPDIR/err.txt" + printf '' > "$stdout_file" + printf 'Error: database is locked\n' > "$stderr_file" + stub="$(_stub_sqlite3_fixed "$stdout_file" "$stderr_file" 5)" + outfile="$BATS_TEST_TMPDIR/outcome" + PATH="$stub:$PATH" + run --separate-stderr bash -c "set -euo pipefail; source '$SCRIPTS/lib/storage.sh'; AGMSG_SQLITE_OUTCOME_FILE='$outfile' agmsg_sqlite ':memory:' 'irrelevant, the stub ignores it;'" + [ "$status" -eq 5 ] + [ "$(cat "$outfile")" = busy ] + [[ "$stderr" == *"Error: database is locked"* ]] +} diff --git a/tests/test_watch.bats b/tests/test_watch.bats index b50f830ab..b4168caa2 100644 --- a/tests/test_watch.bats +++ b/tests/test_watch.bats @@ -944,6 +944,53 @@ _record_handover_events() { done } +# --- argv-length regression (#777) -------------------------------------- +# +# watch.sh used to embed the whole page of `storage_watch_after` rows into +# ONE argv element for `sqlite3 ':memory:' ""`, and its failure +# was swallowed by a trailing `2>/dev/null || true` -- so ROWS silently +# became empty, FINAL_CURSOR never got set, and the read cursor never +# advanced. The same backlog would then fail identically on every following +# poll: not a one-off skip, a stall. +# +# 100 messages of ~2000 bytes each is about 200,000 bytes of body alone, +# well past Linux's MAX_ARG_STRLEN (131,072 bytes; smaller still on +# Windows/macOS). Sent BEFORE the watcher starts, so its very first poll has +# to scan and embed the entire backlog in one statement -- the shape the bug +# needed, rather than many small pages that would each stay under the +# ceiling on their own. +@test "watch: a backlog large enough to exceed the OS argv ceiling still delivers and advances the cursor (#777)" { + skip_on_windows "watcher background launch under Git Bash (#182)" + local sid="sess-argv-backlog" + local out="$TEST_SKILL_DIR/argv-backlog.log" + + bulk_send_direct team bob alice 100 2000 WBIG + + AGMSG_WATCH_INTERVAL=1 bash "$SCRIPTS/watch.sh" "$sid" "$PROJ" claude-code >"$out" 2>/dev/null 3>&- 4>&- & + local w=$! + _wait_for_file_contains "$out" "WBIG-99-" || { kill "$w" 2>/dev/null || true; false; } + + # Cursor advancement is a SEPARATE step that runs after every row in this + # poll has already been printed (storage_read_cursor_consume, embedding all + # 100 delivered ids in its own statement) -- killing the watcher the instant + # the last line lands, the way the plain burst test (#245) does, races that + # step under this much data. Poll for it instead, same as "watch: restart + # delivers messages that arrived while the watcher was down" above. + local i cursor + for i in $(seq 1 100); do + cursor=$(_read_cursor team alice 2>/dev/null || echo 0) + [ "${cursor:-0}" -gt 0 ] && break + sleep 0.1 + done + kill "$w" 2>/dev/null || true + wait "$w" 2>/dev/null || true + + grep -q "WBIG-0-" "$out" + grep -q "WBIG-99-" "$out" + # Not stuck: the store-owned cursor moved past where it started (0). + [ "${cursor:-0}" -gt 0 ] +} + @test "watch: empty session_id gets a generated fallback instead of a Usage error (#236)" { local out="$BATS_TEST_TMPDIR/empty-sid.out" AGMSG_WATCH_INTERVAL=1 bash "$SCRIPTS/watch.sh" "" "$PROJ" claude-code alice >"$out" 2>&1 3>&- 4>&- & diff --git a/tests/test_watch_once.bats b/tests/test_watch_once.bats index 2ce8a9532..bba75cb8f 100644 --- a/tests/test_watch_once.bats +++ b/tests/test_watch_once.bats @@ -105,6 +105,27 @@ _assert_startup_was_delayed() { [[ "$output" =~ "hello pending" ]] } +# --- argv-length regression (#777) -------------------------------------- +# +# This pair's unread ids used to be embedded into ONE argv element for +# `sqlite3 ':memory:' ""`, with the failure swallowed by a +# trailing `2>/dev/null || true` -- so `ids` silently became empty and the +# `[ -n "$ids" ] || continue` a few lines later skipped the whole team every +# single poll, never marking anything read (this script never does) and +# never reporting it pending either. +# +# 100 messages of ~2000 bytes each is about 200,000 bytes of body alone, +# well past Linux's MAX_ARG_STRLEN (131,072 bytes; smaller still on +# Windows/macOS). +@test "watch-once: a backlog large enough to exceed the OS argv ceiling still reports pending (#777)" { + bulk_send_direct team bob alice 100 2000 WOBIG + + run bash "$TYPES/codex/watch-once.sh" "$PROJ" codex --name alice --team team --timeout 2 --interval 1 + [ "$status" -eq 0 ] + grep -qF -- "status=pending" <<< "$output" + [[ "$output" =~ "count=100" ]] +} + @test "watch-once: ignores messages already read by inbox.sh" { bash "$SCRIPTS/send.sh" team bob alice "read already" >/dev/null bash "$SCRIPTS/inbox.sh" team alice >/dev/null From ff4a0e48c8ad790d9168ef266c9cacdd88eb5a91 Mon Sep 17 00:00:00 2001 From: hyamakawa7 Date: Tue, 15 Sep 2026 04:14:55 +0900 Subject: [PATCH 16/34] fix(codex-shim): pass non-remote subcommands through (#590) The shim decides which codex subcommands bypass the monitor bridge from a fixed list. Subcommands missing from it -- such as `codex plugin marketplace add ...` -- fell through to the bridge path, got --remote injected, and were rejected by codex. Add the non-remote subcommands and aliases codex-cli now has (e, a, plugin, remote-control, update, doctor, cloud, exec-server, features), document plugin in the monitor guide, and cover each in the passthrough test. mcp-server is deliberately not on the list: codex-cli has no such subcommand (MCP lives under `codex mcp`), and because codex accepts a positional prompt, an unknown first word starts an interactive session -- which must still go through the bridge. Co-authored-by: hyamakawa7 --- docs/codex-monitor-beta.md | 1 + scripts/drivers/types/codex/codex-shim.sh | 7 +++- tests/test_codex_shim.bats | 46 ++++++++++++++++++++++- 3 files changed, 51 insertions(+), 3 deletions(-) diff --git a/docs/codex-monitor-beta.md b/docs/codex-monitor-beta.md index 4fa3ba1ef..9562e0efd 100644 --- a/docs/codex-monitor-beta.md +++ b/docs/codex-monitor-beta.md @@ -105,6 +105,7 @@ codex exec ... codex app-server ... codex login codex logout +codex plugin ... ``` The shim also passes through when the current project is not in Codex monitor diff --git a/scripts/drivers/types/codex/codex-shim.sh b/scripts/drivers/types/codex/codex-shim.sh index c88695795..ea4d4dbbf 100755 --- a/scripts/drivers/types/codex/codex-shim.sh +++ b/scripts/drivers/types/codex/codex-shim.sh @@ -185,7 +185,12 @@ case "$command_name" in AGMSG_REAL_CODEX="$real_codex" exec "$monitor_cmd" --project "$project" --codex-command resume -- fi ;; - app-server|exec|login|logout|mcp|completion|debug|apply|review|sandbox|help|--help|-h|version|--version|-V) + # Keep known non-remote subcommands out of the monitor bridge. The catch-all + # below is intentional: prompt strings and remote-aware session commands + # (fork/archive/delete/unarchive) must still reach the shared app-server. + # Add newly introduced non-remote Codex subcommands to this list. + app-server|exec|e|login|logout|mcp|plugin|remote-control|completion|update|doctor|\ + cloud|exec-server|features|debug|apply|a|review|sandbox|help|--help|-h|version|--version|-V) exec "$real_codex" "$@" ;; *) diff --git a/tests/test_codex_shim.bats b/tests/test_codex_shim.bats index e3daad35c..526714da4 100644 --- a/tests/test_codex_shim.bats +++ b/tests/test_codex_shim.bats @@ -76,14 +76,56 @@ teardown() { @test "codex shim: noninteractive codex subcommands pass through even in monitor mode" { bash "$SCRIPTS/delivery.sh" set monitor codex "$TEST_PROJECT" >/dev/null - AGMSG_REAL_CODEX="$FAKE_CODEX" AGMSG_CODEX_MONITOR_CMD="$FAKE_MONITOR" \ - run bash "$TYPES/codex/codex-shim.sh" exec echo hi + run bash -c 'cd "$TEST_PROJECT" && AGMSG_REAL_CODEX="$FAKE_CODEX" AGMSG_CODEX_MONITOR_CMD="$FAKE_MONITOR" bash "$TYPES/codex/codex-shim.sh" exec echo hi' [ "$status" -eq 0 ] grep -q "real-codex " "$CALL_LOG" ! grep -q "^monitor" "$CALL_LOG" } +@test "codex shim: plugin marketplace commands pass through even in monitor mode" { + bash "$SCRIPTS/delivery.sh" set monitor codex "$TEST_PROJECT" >/dev/null + + run bash -c 'cd "$TEST_PROJECT" && AGMSG_REAL_CODEX="$FAKE_CODEX" AGMSG_CODEX_MONITOR_CMD="$FAKE_MONITOR" bash "$TYPES/codex/codex-shim.sh" plugin marketplace add owner/repository' + + [ "$status" -eq 0 ] + grep -Fq "real-codex " "$CALL_LOG" + ! grep -q "^monitor" "$CALL_LOG" +} + +@test "codex shim: known non-remote subcommands and aliases bypass the monitor bridge" { + bash "$SCRIPTS/delivery.sh" set monitor codex "$TEST_PROJECT" >/dev/null + + local subcommand + for subcommand in remote-control update doctor cloud exec-server features e a; do + : > "$CALL_LOG" + + run bash -c 'cd "$TEST_PROJECT" && AGMSG_REAL_CODEX="$FAKE_CODEX" AGMSG_CODEX_MONITOR_CMD="$FAKE_MONITOR" bash "$TYPES/codex/codex-shim.sh" "$1" test-arg' _ "$subcommand" + + [ "$status" -eq 0 ] + grep -Fq "real-codex <$subcommand> " "$CALL_LOG" + ! grep -q "^monitor" "$CALL_LOG" + done +} + +@test "codex shim: remote-aware session commands continue through the monitor bridge" { + bash "$SCRIPTS/delivery.sh" set monitor codex "$TEST_PROJECT" >/dev/null + + local subcommand + for subcommand in fork archive delete unarchive; do + : > "$CALL_LOG" + + run bash -c 'cd "$TEST_PROJECT" && AGMSG_REAL_CODEX="$FAKE_CODEX" AGMSG_CODEX_MONITOR_CMD="$FAKE_MONITOR" bash "$TYPES/codex/codex-shim.sh" "$1" test-session' _ "$subcommand" + + [ "$status" -eq 0 ] + grep -Fq "monitor real=$FAKE_CODEX <--project> <$TEST_PROJECT> <--codex-command> <--> <$subcommand> " "$CALL_LOG" || { + echo "unexpected routing for $subcommand: $(cat "$CALL_LOG")" >&3 + false + } + ! grep -q "^real-codex" "$CALL_LOG" + done +} + @test "codex shim: --cd project is used for monitor detection" { bash "$SCRIPTS/delivery.sh" set monitor codex "$TEST_PROJECT" >/dev/null From f9061f074e6124f750b7aa5724e52682943b6ba5 Mon Sep 17 00:00:00 2001 From: fujibee Date: Mon, 14 Sep 2026 14:53:54 -0700 Subject: [PATCH 17/34] feat: terminal driver v1 (integration/terminal-driver-v1 into main) (#1240) --- .github/enforced-assertions-baseline | 2 +- .github/errexit-status-reads-baseline | 1 + .github/herdr-cli-routing-allowlist | 19 + .github/scripts/bats-file-seconds.tsv | 146 + .github/scripts/check-errexit-status-reads.sh | 481 +++ .github/scripts/check-herdr-cli-routing.sh | 95 + .github/scripts/check-unguarded-env-reads.sh | 142 + .github/scripts/shard-tests.sh | 244 +- .github/unguarded-env-reads-baseline | 1 + .github/workflows/tests.yml | 80 +- .gitignore | 1 + CHANGELOG.md | 28 +- README.md | 9 +- SKILL.md | 547 ++- cliff.toml | 3 + docs/agent-types.md | 32 +- docs/antigravity-monitor-beta.md | 318 ++ docs/design/agmsgd-rfc.ja.md | 240 ++ docs/design/agmsgd-rfc.md | 240 ++ .../antigravity-conversation-registration.md | 360 ++ docs/design/antigravity-monitor-bridge.md | 303 ++ docs/design/antigravity-tui-pty-monitor.md | 190 + install.sh | 143 +- scripts/actas-claim.sh | 62 +- scripts/arrange.sh | 77 + scripts/check-inbox.sh | 117 +- scripts/delivery.sh | 67 +- scripts/despawn.sh | 214 +- scripts/doctor.sh | 66 +- .../drivers/storage/jsonl-unread.duckdb.sql | 6 +- scripts/drivers/storage/jsonl.sh | 2 +- scripts/drivers/terminals/herdr/README.md | 28 + scripts/drivers/terminals/herdr/ops.sh | 1387 +++++++ scripts/drivers/terminals/herdr/terminal.conf | 9 + scripts/drivers/terminals/plain/README.md | 31 + .../plain/adapters/iterm.applescript | 44 + .../plain/adapters/terminal.applescript | 38 + scripts/drivers/terminals/plain/ops.sh | 439 +++ scripts/drivers/terminals/plain/terminal.conf | 8 + scripts/drivers/terminals/tmux/README.md | 22 + scripts/drivers/terminals/tmux/ops.sh | 780 ++++ scripts/drivers/terminals/tmux/terminal.conf | 8 + scripts/drivers/types/agmsg-app/type.conf | 2 +- .../drivers/types/antigravity/_delivery.sh | 72 +- scripts/drivers/types/antigravity/agy-tui.sh | 74 + .../types/antigravity/antigravity-bridge.mjs | 210 + .../types/antigravity/antigravity-mode.mjs | 19 + .../types/antigravity/antigravity-monitor.sh | 17 + .../types/antigravity/antigravity-resume.sh | 31 + .../antigravity/antigravity-tui-monitor.sh | 21 + .../antigravity/antigravity-tui-supervisor.py | 776 ++++ .../types/antigravity/inbox-transport.sh | 80 + scripts/drivers/types/antigravity/template.md | 251 +- scripts/drivers/types/antigravity/type.conf | 4 +- scripts/drivers/types/claude-code/template.md | 310 +- scripts/drivers/types/claude-code/type.conf | 23 +- scripts/drivers/types/codex/_session-start.sh | 6 + .../types/codex/codex-bridge-launcher.sh | 7 +- scripts/drivers/types/codex/codex-bridge.js | 11 +- scripts/drivers/types/codex/codex-monitor.sh | 10 + .../types/codex/codex-record-session.sh | 3 +- scripts/drivers/types/codex/eligible-pairs.sh | 4 +- scripts/drivers/types/codex/template.md | 293 +- scripts/drivers/types/codex/type.conf | 93 +- scripts/drivers/types/codex/watch-once.sh | 8 +- scripts/drivers/types/copilot/template.md | 251 +- scripts/drivers/types/copilot/type.conf | 2 +- scripts/drivers/types/cursor/_delivery.sh | 18 + scripts/drivers/types/cursor/template.md | 254 +- scripts/drivers/types/cursor/type.conf | 2 +- scripts/drivers/types/gemini/template.md | 251 +- scripts/drivers/types/gemini/type.conf | 6 +- scripts/drivers/types/grok-build/_delivery.sh | 33 +- scripts/drivers/types/grok-build/template.md | 285 +- scripts/drivers/types/grok-build/type.conf | 3 +- scripts/drivers/types/hermes/template.md | 242 +- scripts/drivers/types/hermes/type.conf | 2 +- scripts/drivers/types/opencode/_delivery.sh | 31 + scripts/drivers/types/opencode/template.md | 282 +- scripts/drivers/types/opencode/type.conf | 2 +- scripts/fix.sh | 16 + scripts/history.sh | 13 + scripts/inbox.sh | 31 +- scripts/internal/private-names.mjs | 53 +- scripts/internal/resurrect-panes.sh | 8 +- scripts/join.sh | 44 + scripts/lib/actas-lock.sh | 709 +++- scripts/lib/bridge-read-guard.mjs | 80 + scripts/lib/bridge-read-guard.sh | 31 + scripts/lib/delivery-rulefile.sh | 18 + scripts/lib/detect-cli-type.sh | 55 +- scripts/lib/hooks-json.sh | 50 +- scripts/lib/instance-id.sh | 102 +- scripts/lib/placement-actual-location.sh | 53 + scripts/lib/role-session.sh | 180 +- scripts/lib/self-fix.sh | 146 + scripts/lib/self-identity.sh | 294 ++ scripts/lib/self-name.sh | 194 + scripts/lib/self-proof.sh | 411 ++ scripts/lib/self-rename.sh | 264 ++ scripts/lib/self-write-lock.sh | 72 + scripts/lib/self-write.sh | 429 +++ scripts/lib/skill-render.sh | 122 + scripts/lib/storage.sh | 4 +- scripts/lib/subscription.sh | 41 +- scripts/lib/team-status.sh | 447 +++ scripts/lib/terminal-context-line.sh | 85 + scripts/lib/terminal-registry.sh | 1414 +++++++ scripts/lib/token-locate.sh | 162 + scripts/lib/type-registry.sh | 26 +- scripts/lib/watch-stuck-map.sh | 91 + scripts/peek.sh | 161 + scripts/placement-collisions.sh | 263 ++ scripts/poke.sh | 107 + scripts/remote.sh | 11 +- scripts/reset.sh | 25 + scripts/send.sh | 65 +- scripts/session-start.sh | 83 +- scripts/spawn.sh | 667 +++- scripts/team.sh | 232 +- scripts/watch.sh | 673 +++- scripts/where.sh | 99 + server/test/sync-client.integration.test.ts | 21 +- tests/antigravity_bridge.test.mjs | 227 ++ tests/antigravity_tui_supervisor.test.mjs | 962 +++++ .../agy-1.1.27-screen-transcripts.json | 40 + tests/fixtures/fake-antigravity.mjs | 38 + tests/helpers/mock_remote_server.py | 50 + tests/private_names.test.mjs | 64 +- tests/remote_sync_engine.test.mjs | 18 +- tests/test_actas_integration.bats | 131 +- tests/test_actas_lock.bats | 690 +++- tests/test_antigravity_resume.bats | 82 + tests/test_antigravity_transport.bats | 213 + tests/test_capability_docs.bats | 178 + tests/test_ci_sharding.bats | 88 + tests/test_claude_template.bats | 69 +- tests/test_codex_bridge.bats | 5 +- tests/test_codex_bridge_launcher.bats | 44 +- tests/test_codex_monitor.bats | 25 + tests/test_delivery.bats | 618 ++- tests/test_despawn.bats | 676 +++- tests/test_dispatch.bats | 43 + tests/test_doctor.bats | 52 +- tests/test_engine_inherited_fds.bats | 47 + tests/test_harness_self_name_off.bats | 46 + tests/test_helper.bash | 125 + tests/test_herdr_cli_routing.bats | 134 + tests/test_inbox.bats | 60 +- tests/test_install.bats | 261 +- tests/test_instance_id.bats | 150 + tests/test_locator.bats | 135 + tests/test_messaging.bats | 48 + tests/test_migrate_team_store.bats | 4 +- tests/test_peek_poke.bats | 809 ++++ tests/test_placement_actual_location.bats | 70 + tests/test_placement_collisions.bats | 242 ++ tests/test_remote.bats | 22 +- tests/test_remote_sync_engine.bats | 1 + tests/test_reset_role_session.bats | 115 + tests/test_resume_seat_guard.bats | 2 +- tests/test_role_session.bats | 37 +- tests/test_roster_journal.bats | 8 +- tests/test_self_fix.bats | 172 + tests/test_self_identity.bats | 490 +++ tests/test_self_name.bats | 616 +++ tests/test_self_proof.bats | 895 +++++ tests/test_self_rename.bats | 323 ++ tests/test_self_write.bats | 502 +++ tests/test_self_write_lock.bats | 518 +++ tests/test_session_start_terminal_line.bats | 78 + tests/test_spawn.bats | 1111 +++++- tests/test_sweep_enumeration.bats | 432 +++ tests/test_team.bats | 119 +- tests/test_team_guidance_usage.bats | 63 + tests/test_team_list.bats | 12 +- tests/test_team_status.bats | 676 ++++ tests/test_terminal_context_line.bats | 87 + tests/test_terminal_registry.bats | 3428 +++++++++++++++++ tests/test_token_locate.bats | 164 + tests/test_type_registry.bats | 253 +- tests/test_unguarded_env_reads.bats | 189 + tests/test_watch.bats | 673 +++- tests/test_watch_once.bats | 35 + tests/test_watch_stuck_map.bats | 159 + tests/test_where.bats | 128 + uninstall.sh | 28 +- 187 files changed, 33207 insertions(+), 3644 deletions(-) create mode 100644 .github/errexit-status-reads-baseline create mode 100644 .github/herdr-cli-routing-allowlist create mode 100644 .github/scripts/bats-file-seconds.tsv create mode 100755 .github/scripts/check-errexit-status-reads.sh create mode 100755 .github/scripts/check-herdr-cli-routing.sh create mode 100755 .github/scripts/check-unguarded-env-reads.sh create mode 100644 .github/unguarded-env-reads-baseline create mode 100644 docs/antigravity-monitor-beta.md create mode 100644 docs/design/agmsgd-rfc.ja.md create mode 100644 docs/design/agmsgd-rfc.md create mode 100644 docs/design/antigravity-conversation-registration.md create mode 100644 docs/design/antigravity-monitor-bridge.md create mode 100644 docs/design/antigravity-tui-pty-monitor.md create mode 100755 scripts/arrange.sh create mode 100644 scripts/drivers/terminals/herdr/README.md create mode 100644 scripts/drivers/terminals/herdr/ops.sh create mode 100644 scripts/drivers/terminals/herdr/terminal.conf create mode 100644 scripts/drivers/terminals/plain/README.md create mode 100644 scripts/drivers/terminals/plain/adapters/iterm.applescript create mode 100644 scripts/drivers/terminals/plain/adapters/terminal.applescript create mode 100644 scripts/drivers/terminals/plain/ops.sh create mode 100644 scripts/drivers/terminals/plain/terminal.conf create mode 100644 scripts/drivers/terminals/tmux/README.md create mode 100644 scripts/drivers/terminals/tmux/ops.sh create mode 100644 scripts/drivers/terminals/tmux/terminal.conf create mode 100644 scripts/drivers/types/antigravity/agy-tui.sh create mode 100644 scripts/drivers/types/antigravity/antigravity-bridge.mjs create mode 100644 scripts/drivers/types/antigravity/antigravity-mode.mjs create mode 100644 scripts/drivers/types/antigravity/antigravity-monitor.sh create mode 100755 scripts/drivers/types/antigravity/antigravity-resume.sh create mode 100644 scripts/drivers/types/antigravity/antigravity-tui-monitor.sh create mode 100644 scripts/drivers/types/antigravity/antigravity-tui-supervisor.py create mode 100644 scripts/drivers/types/antigravity/inbox-transport.sh create mode 100755 scripts/fix.sh create mode 100644 scripts/lib/bridge-read-guard.mjs create mode 100644 scripts/lib/bridge-read-guard.sh create mode 100644 scripts/lib/placement-actual-location.sh create mode 100644 scripts/lib/self-fix.sh create mode 100644 scripts/lib/self-identity.sh create mode 100644 scripts/lib/self-name.sh create mode 100644 scripts/lib/self-proof.sh create mode 100644 scripts/lib/self-rename.sh create mode 100644 scripts/lib/self-write-lock.sh create mode 100644 scripts/lib/self-write.sh create mode 100644 scripts/lib/skill-render.sh create mode 100644 scripts/lib/team-status.sh create mode 100644 scripts/lib/terminal-context-line.sh create mode 100644 scripts/lib/terminal-registry.sh create mode 100644 scripts/lib/token-locate.sh create mode 100644 scripts/lib/watch-stuck-map.sh create mode 100755 scripts/peek.sh create mode 100755 scripts/placement-collisions.sh create mode 100755 scripts/poke.sh create mode 100755 scripts/where.sh create mode 100644 tests/antigravity_bridge.test.mjs create mode 100644 tests/antigravity_tui_supervisor.test.mjs create mode 100644 tests/fixtures/agy-1.1.27-screen-transcripts.json create mode 100644 tests/fixtures/fake-antigravity.mjs create mode 100644 tests/test_antigravity_resume.bats create mode 100644 tests/test_antigravity_transport.bats create mode 100644 tests/test_capability_docs.bats create mode 100644 tests/test_harness_self_name_off.bats create mode 100644 tests/test_herdr_cli_routing.bats create mode 100644 tests/test_locator.bats create mode 100644 tests/test_peek_poke.bats create mode 100644 tests/test_placement_actual_location.bats create mode 100644 tests/test_placement_collisions.bats create mode 100644 tests/test_reset_role_session.bats create mode 100644 tests/test_self_fix.bats create mode 100644 tests/test_self_identity.bats create mode 100644 tests/test_self_name.bats create mode 100644 tests/test_self_proof.bats create mode 100644 tests/test_self_rename.bats create mode 100644 tests/test_self_write.bats create mode 100644 tests/test_self_write_lock.bats create mode 100644 tests/test_session_start_terminal_line.bats create mode 100644 tests/test_sweep_enumeration.bats create mode 100644 tests/test_team_guidance_usage.bats create mode 100644 tests/test_team_status.bats create mode 100644 tests/test_terminal_context_line.bats create mode 100644 tests/test_terminal_registry.bats create mode 100644 tests/test_token_locate.bats create mode 100644 tests/test_unguarded_env_reads.bats create mode 100644 tests/test_watch_stuck_map.bats create mode 100644 tests/test_where.bats diff --git a/.github/enforced-assertions-baseline b/.github/enforced-assertions-baseline index 7d71bf4ad..104eee38f 100644 --- a/.github/enforced-assertions-baseline +++ b/.github/enforced-assertions-baseline @@ -1 +1 @@ -635 +626 diff --git a/.github/errexit-status-reads-baseline b/.github/errexit-status-reads-baseline new file mode 100644 index 000000000..573541ac9 --- /dev/null +++ b/.github/errexit-status-reads-baseline @@ -0,0 +1 @@ +0 diff --git a/.github/herdr-cli-routing-allowlist b/.github/herdr-cli-routing-allowlist new file mode 100644 index 000000000..cd45ac004 --- /dev/null +++ b/.github/herdr-cli-routing-allowlist @@ -0,0 +1,19 @@ +# Functions in scripts/drivers/terminals/herdr/ops.sh that call the `herdr` CLI +# directly, with the exact number of such calls. Checked by +# .github/scripts/check-herdr-cli-routing.sh in both directions. +# +# Every function here is instance-wide or deliberately ambient: it lists or +# creates in the instance the environment names, and takes no pane id whose +# socket could name a different one. A function that operates ON a pane id is +# not allowed here -- it routes through `_herdr_cli ...`, which sets +# HERDR_SOCKET_PATH from a socket-qualified id. (`_herdr_cli` itself calls +# `herdr "$@"` with no literal subcommand word, so the position rule never +# counts it: it is the one routed call by construction.) +# +# +_herdr_pane_for_session 1 +terminal_describe 2 +terminal_detect 1 +terminal_find_by_label 1 +terminal_spawn 2 +terminal_enumerate_panes 2 diff --git a/.github/scripts/bats-file-seconds.tsv b/.github/scripts/bats-file-seconds.tsv new file mode 100644 index 000000000..d79312125 --- /dev/null +++ b/.github/scripts/bats-file-seconds.tsv @@ -0,0 +1,146 @@ +# Per-file real macOS bats wall time (seconds), used by shard-tests.sh to +# weight the CI partition by measured cost instead of @test count (#1243). +# @test count is a loose, occasionally blind proxy for a file's real cost: +# a file whose time is almost entirely waiting (background processes, poll +# loops) can carry a tiny count while dominating its shard's wall clock, and +# a file that is merely large in @test count is not necessarily slow. This +# table sidesteps the proxy by using each file's own measured time directly. +# +# MEASURED AGAINST: GitHub Actions run 34845597032 (release-candidate draft +# PR #1240, tests.yml bats job, macos-latest, 5 shards, 2026-09-14, the tree +# after #1105/#1231/#1234/#1237/#1239/#1241/#1245 landed). Every shard in +# that run completed (none timed out), so every file below has a real +# file_end record -- summed from the run's 5 bats-timings-macos-latest-* +# artifacts (file_end rows only, macOS only). +# +# A file with no row here (renamed, or added after this measurement) is +# estimated at this table's own average seconds/file by shard-tests.sh, +# never treated as zero-cost or dropped from the partition -- coverage does +# not depend on this table being current. Refresh by re-running this +# extraction against a green run's bats-timings artifacts once the tree +# drifts enough that the balance this buys degrades (watched by +# tests/test_ci_sharding.bats's balance-ratio assertion, not by a schedule). +# +# Columns: basenameseconds. Sorted by basename for a legible diff. +test_actas_integration.bats 65 +test_actas_lock.bats 35 +test_antigravity_resume.bats 1 +test_antigravity_transport.bats 12 +test_api.bats 44 +test_app_release_signing.bats 1 +test_apply_fail_stderr.bats 0 +test_bin_agmsg.bats 2 +test_binding_mode.bats 3 +test_boot_command.bats 10 +test_bump_app_version.bats 0 +test_capability_docs.bats 2 +test_ci_forensics.bats 1 +test_ci_sharding.bats 69 +test_ci_workflow.bats 0 +test_claude_template.bats 1 +test_close_fds.bats 0 +test_cmdline_path_match.bats 1 +test_codex_bridge.bats 199 +test_codex_bridge_launcher.bats 281 +test_codex_monitor.bats 9 +test_codex_resume.bats 10 +test_codex_shim.bats 20 +test_compat.bats 3 +test_compat_posix.bats 1 +test_config.bats 2 +test_cut_release.bats 5 +test_delivery.bats 432 +test_despawn.bats 137 +test_dispatch.bats 75 +test_doctor.bats 97 +test_endpoint_scheme.bats 1 +test_endpoint_table_node.bats 0 +test_enforced_assertions.bats 2 +test_engine_inherited_fds.bats 21 +test_export.bats 37 +test_harness_self_name_off.bats 1 +test_hash.bats 4 +test_herdr_cli_routing.bats 3 +test_identity_path_forms.bats 6 +test_inbox.bats 114 +test_install.bats 109 +test_instance_id.bats 30 +test_jsonl_remote_sync.bats 44 +test_key.bats 72 +test_legacy_mirror.bats 11 +test_local_quoting.bats 1 +test_local_team_ids.bats 11 +test_locator.bats 5 +test_messaging.bats 124 +test_migrate_team_store.bats 151 +test_node_resolve.bats 1 +test_peek_poke.bats 22 +test_perf_harness.bats 12 +test_placement_actual_location.bats 2 +test_placement_collisions.bats 38 +test_plugin_registry.bats 7 +test_printed_command_paths.bats 10 +test_refute.bats 1 +test_registry_lock.bats 8 +test_release_ci_skip.bats 1 +test_release_dist_tag.bats 1 +test_remote.bats 453 +test_remote_curl_config_paths.bats 5 +test_remote_curl_stderr.bats 3 +test_remote_curl_stderr_get.bats 6 +test_remote_engine_start_refusal.bats 660 +test_remote_forget.bats 11 +test_remote_header_sink.bats 4 +test_remote_refusal.bats 18 +test_remote_setup_doc.bats 8 +test_remote_status_liveness.bats 346 +test_remote_sync.bats 328 +test_remote_sync_apply_blob.bats 5 +test_remote_sync_ca_bundle.bats 1 +test_remote_sync_driver_input.bats 0 +test_remote_sync_engine.bats 12 +test_remote_sync_reject_envelope.bats 5 +test_require_python3.bats 5 +test_reset_role_session.bats 13 +test_resolve_project.bats 35 +test_resume_seat_guard.bats 19 +test_resurrect_panes.bats 11 +test_role_session.bats 14 +test_roster_journal.bats 99 +test_self_fix.bats 13 +test_self_identity.bats 24 +test_self_name.bats 149 +test_self_proof.bats 42 +test_self_rename.bats 30 +test_self_write.bats 46 +test_self_write_lock.bats 36 +test_session_start_terminal_line.bats 12 +test_setup.bats 1 +test_shquote.bats 1 +test_spawn.bats 477 +test_spawn_fd_guard.bats 1 +test_spawn_options.bats 2 +test_sqlite_crlf.bats 9 +test_sqlite_sync_jq_binary.bats 5 +test_storage.bats 37 +test_storage_contract.bats 44 +test_sweep_enumeration.bats 9 +test_sync_autostart.bats 29 +test_sync_cipher.bats 7 +test_team.bats 198 +test_team_guidance_usage.bats 1 +test_team_list.bats 28 +test_team_status.bats 14 +test_teardown_reap.bats 3 +test_terminal_context_line.bats 2 +test_terminal_registry.bats 81 +test_token_locate.bats 3 +test_transcript_exists.bats 2 +test_type_registry.bats 36 +test_unguarded_env_reads.bats 2 +test_wait_helpers.bats 40 +test_watch.bats 317 +test_watch_install_changed.bats 24 +test_watch_once.bats 91 +test_watch_stuck_map.bats 3 +test_where.bats 1 diff --git a/.github/scripts/check-errexit-status-reads.sh b/.github/scripts/check-errexit-status-reads.sh new file mode 100755 index 000000000..eeb78e7ad --- /dev/null +++ b/.github/scripts/check-errexit-status-reads.sh @@ -0,0 +1,481 @@ +#!/usr/bin/env bash +# +# Fail when a script grows a `$?` read that errexit never lets it reach — or +# that it reaches only to find a 0 that means nothing. +# +# Measured, not assumed. Each row run as `bash -c 'set -e;
'` on both +# interpreters this project ships against: +# +# bash 3.2.57 bash 5.3.15 +# x=$(false); rc=$?; echo $rc shell dies shell dies +# local x=$(false); rc=$?; echo $rc rc=0, survives rc=0, survives +# declare x=$(false); rc=$? rc=0, survives rc=0, survives +# export x=$(false); rc=$? rc=0, survives rc=0, survives +# x=$(false) || rc=$? rc=1 rc=1 +# if x=$(false); then :; fi not fatal not fatal +# f() { . failing.sh; rc=$?; } shell dies rc=1, survives <- differs +# f() { . failing.sh || rc=$?; } shell dies rc=1, survives <- differs +# +# Three ways to be wrong, one shape to look for — a status read that follows +# something errexit already decided: +# +# [bare] x=$(cmd) the assignment's status IS the substitution's, +# rc=$? so a non-zero one kills the shell HERE. The +# next line is unreachable; the handler it feeds +# has never run. +# +# [decl] local x=$(cmd) `local`/`declare`/`typeset`/`export`/`readonly` +# rc=$? is a builtin whose own status wins. The shell +# survives and `rc` is ALWAYS 0 — a handler that +# reads as present and can never fire. This is +# the quiet one; nothing crashes. +# +# [source] . file On bash 3.2 a failing command at the top of a +# rc=$? sourced file fires the CALLER's errexit, and it +# does so EVEN with `|| rc=$?` on the source line +# (measured; the `||` does not save 3.2). macOS +# /bin/bash is 3.2, so this is a macOS-only death +# that passes every Linux run. +# +# The accepted fix for all three is the codebase's two-line lift — see +# `agmsg_terminal_load` in scripts/lib/terminal-registry.sh: +# +# local rc=0 restore_e=0 +# case $- in *e*) restore_e=1 ;; esac +# set +e +# x=$(cmd) # or `. file` +# rc=$? +# [ "$restore_e" = 1 ] && set -e +# +# so a statement sitting between `set +e` and `set -e` is NOT flagged: that is +# the fix, not the defect. `|| rc=$?` is not flagged either for [bare]/[decl] +# (measured correct on both shells) — but it does NOT clear [source]. +# +# WHAT IS EXCLUDED, and why: +# - anything between `set +e` and the next `set -e`: errexit is lifted, which +# is the whole point of lifting it +# - a statement carrying `||` or `&&`: explicit control (except [source]) +# - the condition of `if` / `while` / `until`: errexit does not apply there +# +# The baseline is a COUNT, not a file:line list, so moving code between files +# does not produce a spurious failure. It may only go down. +# +# WHY THIS CANNOT PASS BY FAILING TO LOOK: before it reports anything about the +# tree, it runs the same scanner over a fixture holding one known-bad instance +# of each kind and requires all three back. A regex that stops matching — a +# refactor, a quoting change, a wrong path — then exits 2 (could not answer) +# instead of 0 (nothing found). "Zero" is only ever printed by a scanner that +# has just proved it can find one. + +set -u + +ROOT="$(cd "$(dirname "$0")/../.." && pwd)" +BASELINE_FILE="${AGMSG_ERREXIT_BASELINE:-$ROOT/.github/errexit-status-reads-baseline}" +SCAN_DIR="${1:-$ROOT/scripts}" + +scan() { + python3 - "$1" <<'PY' +import re, sys, pathlib + +ASSIGN = re.compile(r'^(?Plocal|declare|typeset|export|readonly)?\s*' + r'(?P[A-Za-z_][A-Za-z0-9_]*)=(?P.*)$', re.S) +SOURCEC = re.compile(r'^(\.|source)\s+\S') +# A status READ is an assignment whose whole value is `$?` -- `rc=$?`, +# `local rc=$?`. A statement that merely CONTAINS `$?` is not one: the +# `|| vrc=$?` on a guarded assignment is that assignment's own handling, +# and reporting the line before it (herdr/ops.sh:60) was a false positive. +STATUS = re.compile(r'^(local|declare|typeset|export|readonly)?\s*[A-Za-z_][A-Za-z0-9_]*=\$\?\s*$') +COND = re.compile(r'^(if|while|until|elif)\b') +SETPLUS = re.compile(r'^set\s+\+[a-zA-Z]*e') +SETMINUS= re.compile(r'^set\s+-[a-zA-Z]*e') + +def split_statements(text): + """Walk the file once, tracking quote state and $( ) nesting ACROSS LINES. + + Splitting per line is what made this wrong: a SQL string that opens on one + line and closes on another left every `;` between them looking like a + statement separator, so + + x="$(sqlite3 :memory: "SELECT ... LIMIT 1;" 2>/dev/null)" || rc=$? + + was cut in half and its `|| rc=$?` guard was reported as an unguarded bare + assignment. A checker that reports the correct form gets worked around, and + a worked-around checker passes while guarding nothing. + + A command substitution opens a FRESH quoting context even when it appears + inside double quotes -- `"$( ... "inner" ... )"` is one word to bash, and + the inner quotes are the substitution's, not the outer string's. So the + quote character is pushed on entering `$(` and restored on the matching + `)`. Modelling that as a flat flag is what let the first fix swallow a real + instance: the `"` right after `$(` read as CLOSING the outer string, and + everything after it fell out of the statement.""" + out = [] + buf, start = '', None + line = 1 + q = None # active quote char in the CURRENT context + stack = [] # saved quote chars, one per open $( + i, n = 0, len(text) + while i < n: + c = text[i] + + # A backslash escapes the next character, and a backslash-NEWLINE is a + # line continuation: bash removes both and the statement carries on. + # Splitting there is what hid + # + # . \\ + # "$dir/ops.sh" + # rc=$? + # + # from the source check -- `. \\` and `"$dir/ops.sh"` became two + # statements, so neither was the `rc=$?`'s predecessor and neither + # matched SOURCEC. `source` is one of the three forms this checker + # names, so that hole made a count of zero unprovable for it. Inside + # SINGLE quotes a backslash is literal and does not continue a line, so + # that case is left to the single-quote branch below. + if c == '\\' and q != "'": + nxt = text[i+1] if i + 1 < n else '' + if nxt == '\n': + line += 1 + i += 2 + continue + if nxt: + buf += c + nxt + if start is None: + start = line + i += 2 + continue + + if c == '\n': + here = line + line += 1 + if q is None and not stack: + if buf.strip(): + out.append((start or here, buf.strip())) + buf, start = '', None + else: + buf += c + i += 1 + continue + + # inside single quotes nothing is special but the closing quote + if q == "'": + buf += c + if c == "'": + q = None + i += 1 + continue + + # command substitution opens a new quoting context, double quotes or not + if text[i:i+2] == '$(': + stack.append(q) + q = None + buf += '$(' + if start is None: + start = line + i += 2 + continue + + if c == ')' and stack and q is None: + q = stack.pop() + buf += c + i += 1 + continue + + if q == '"': + buf += c + if c == '"' and text[i-1] != '\\': + q = None + i += 1 + continue + + # unquoted, at some $( depth or none + if c in ('"', "'"): + q = c + buf += c + if start is None: + start = line + i += 1 + continue + + # `#` opens a comment when it STARTS A WORD -- at the beginning of a + # statement or after whitespace. Requiring the statement to be empty + # was not merely incomplete, it was actively dangerous: a TRAILING + # comment stayed in the text, and the apostrophe in one (`review's`) read + # as an opening single quote and swallowed everything to the next one. + # That silently disabled the sql_bare control several functions later, + # so the file reported three findings instead of four and still looked + # healthy. A checker whose own controls can be switched off by a + # comment is not measuring anything. + # + # `${x#f}` and `$#` are not comments and are not caught here: neither + # follows whitespace. + if c == '#' and (not buf or buf[-1].isspace()) and not stack: + while i < n and text[i] != '\n': + i += 1 + continue + + if c == ';' and not stack: + if buf.strip(): + out.append((start or line, buf.strip())) + buf, start = '', None + i += 1 + continue + + buf += c + if start is None and c.strip(): + start = line + i += 1 + + if buf.strip(): + out.append((start or line, buf.strip())) + return out + +rows = [] +for f in sorted(pathlib.Path(sys.argv[1]).rglob('*.sh')): + lifted = False + prev = None + for n, st in split_statements(f.read_text(errors='replace')): + if SETPLUS.match(st): + lifted = True; prev = (n, st); continue + if SETMINUS.match(st): + lifted = False; prev = (n, st); continue + + if prev and STATUS.match(st) and not lifted: + pn, ps = prev + if not COND.match(ps): + m = ASSIGN.match(ps) + guarded = re.search(r'\|\||&&', ps) + kind = None + if m and ('$(' in m.group('rhs') or '`' in m.group('rhs')): + if not guarded: + kind = 'decl' if m.group('decl') else 'bare' + elif SOURCEC.match(ps): + kind = 'source' + if kind: + flat = ' '.join(ps.split()) + rows.append(f"{f}:{n}: [{kind}] {flat[:60]} -> {' '.join(st.split())[:40]}") + prev = (n, st) + +for r in rows: + print(r) +PY +} + +# ---- what the controls DO and DO NOT cover --------------------------------- +# +# Each hole below was found by a reviewer, not by this file, so the list is +# written down: a count of zero from the tree means "not present" only for the +# syntax the controls actually exercise. +# +# COVERED (a control exists and is pinned by name or by kind): +# `;` as a separator bare_case / decl_case +# single- and double-quoted strings all controls +# `$( )` nesting, incl. quotes inside it sql_guarded_case / sql_bare_case +# a quoted string spanning several lines sql_bare_case (pinned by name) +# backslash-newline continuation source_continued_case, pinned on +# the path it sources +# a TRAILING comment, incl. one holding +# an apostrophe source_continued_case's own +# header comment carries one; if the +# scanner treats it as a quote the +# continuation pin goes red first +# (the swallow starts in that same +# header), and sql_bare with it +# the two ACCEPTED forms staying silent lifted_case / guarded_case / +# sql_guarded_case +# +# NOT COVERED — a status read hidden inside any of these is invisible here, and +# nobody has measured whether the tree contains one: +# backticks `cmd` instead of $( ) the scanner keys on `$(` and on a +# literal backtick in the RHS, but +# no control exercises a backtick +# spanning lines +# heredocs their body is scanned as ordinary +# text, so a `;` or a quote inside +# one can still split a statement +# `{ ...; }` and `( ... )` grouping treated as plain text +# arithmetic `$(( ))` and `(( ))` `$((` enters the `$(` stack and +# its `))` pops only one level +# `case` patterns' `;;` splits, which is harmless today +# but is not asserted anywhere +# `set -e` toggled inside a function or a +# subshell lifting is tracked file-wide, not +# per scope +# +# And one that is NOT a syntax gap, which is why the list above missed it: +# +# a BARE COMMAND OR FUNCTION CALL `f "$x"; rc=$?` is not looked at. +# followed by `rc=$?` -> #1034 The predecessor is only examined +# when it is an assignment with a +# command substitution, or a +# `source` — so the most ordinary +# producer of `$?` is skipped +# entirely. Under `set -e` such a +# call exits the shell before the +# status can be classified. Found by +# review, not by this file. The +# splitter is NOT the problem: it +# already returns the two statements +# correctly; the PREDICATE is what +# is narrow. Widening it, and +# counting what the tree then shows, +# is #1034 — deliberately not done +# here, because the count is unknown +# and would move the baseline. +# +# The two lists are different axes. Everything above the gap line enumerates +# SYNTAX the scanner may mis-split. The entry below it enumerates a SHAPE that +# produces `$?` at all — and that axis had never been written down, which is how +# the commonest shape of the three stayed invisible while the file looked +# thorough. +# +# Adding a control for one of these means moving it up, not deleting the line. +# +# ---- positive control: prove the scanner can still find each known-bad kind -- +control_dir="$(mktemp -d)" +trap 'rm -rf "$control_dir"' EXIT +cat > "$control_dir/control.sh" <<'CTL' +#!/usr/bin/env bash +set -e +bare_case() { + local out + out="$(some_command)" + rc=$? + [ "$rc" -eq 0 ] || return 1 +} +decl_case() { + local out="$(some_command)" + local rc=$? + [ "$rc" -eq 0 ] || return 1 +} +source_case() { + . "$dir/ops.sh" + rc=$? + [ "$rc" -eq 0 ] || return 1 +} +lifted_case() { # the accepted fix — must NOT be reported + set +e + out="$(some_command)" + rc=$? + set -e + [ "$rc" -eq 0 ] || return 1 +} +guarded_case() { # explicit control — must NOT be reported + out="$(some_command)" || rc=$? + [ "${rc:-0}" -eq 0 ] || return 1 +} +sql_guarded_case() { # the false positive that cost a workaround — must NOT + # be reported. The `;` sits inside a double-quoted SQL + # string that OPENS on one line and CLOSES on another. + local out rc=0 + out="$(sqlite3 :memory: " + SELECT json_extract(value,'$.pane_id') + FROM json_each('$j') + LIMIT 1;" 2>/dev/null)" || rc=$? + [ "$rc" -eq 0 ] || return 2 +} +source_continued_case() { # the review's second hole — a source split across a + # backslash-newline. MUST be reported: this is a + # single-line source with a line break in it, and a + # splitter that breaks there reports nothing while + # looking exactly like a clean tree. + local rc=0 + . \ + "$dir/continued-ops.sh" + rc=$? + [ "$rc" -eq 0 ] || return 1 +} +sql_bare_case() { # the same multi-line SQL shape, genuinely unguarded — + # MUST still be reported, or the splitter fix would have + # bought a false negative in place of a false positive. + local out + out="$(sqlite3 :memory: " + SELECT 1;" 2>/dev/null)" + rc=$? + [ "$rc" -eq 0 ] || return 2 +} +CTL +control="$(scan "$control_dir")" +# The multi-line SQL bare case must come back BY NAME, not merely by kind: the +# kinds are covered below, and what this proves is the other direction — that a +# quoted string spanning lines cannot swallow a real instance. +case "$control" in + *continued-ops.sh*) ;; + *) + echo "check-errexit-status-reads: positive control did not report the source" >&2 + echo "split across a backslash-newline. A continuation is ONE statement to" >&2 + echo "bash; a splitter that breaks there finds nothing and looks clean." >&2 + printf '%s\n' "$control" | sed 's/^/ /' >&2 + exit 2 ;; +esac +case "$control" in + *sql_bare_case*|*"SELECT 1"*) ;; + *) + echo "check-errexit-status-reads: positive control did not report the multi-line" >&2 + echo "SQL bare case; the splitter can be made to hide a real one." >&2 + printf '%s\n' "$control" | sed 's/^/ /' >&2 + exit 2 ;; +esac +for kind in bare decl source; do + case "$control" in + *"[$kind]"*) ;; + *) + echo "check-errexit-status-reads: positive control did not report [$kind]." >&2 + echo "The scanner cannot find a form it is supposed to find, so a count of" >&2 + echo "zero from the tree would mean nothing. Fix the scanner, not the tree." >&2 + printf '%s\n' "$control" | sed 's/^/ /' >&2 + exit 2 ;; + esac +done +# and the two correct forms must not be reported, or every fix would look like +# a defect and the baseline could never come down +for bad in lifted_case guarded_case sql_guarded_case; do + case "$control" in + *"$bad"*) + echo "check-errexit-status-reads: positive control reported $bad, which is the" >&2 + echo "accepted form. The scanner would flag the fix; that is not usable." >&2 + exit 2 ;; + esac +done + +# ---- the tree --------------------------------------------------------------- +if [ ! -d "$SCAN_DIR" ] || [ -z "$(find "$SCAN_DIR" -name '*.sh' -print -quit)" ]; then + echo "check-errexit-status-reads: no .sh files under $SCAN_DIR; this is not a clean tree." >&2 + exit 2 +fi + +listing="$(scan "$SCAN_DIR")" +if [ -z "$listing" ]; then + found=0 +else + found="$(printf '%s\n' "$listing" | wc -l | tr -d '[:space:]')" +fi + +baseline="$(tr -d '[:space:]' < "$BASELINE_FILE" 2>/dev/null || echo '')" +case "$baseline" in + ''|*[!0-9]*) + echo "check-errexit-status-reads: no readable baseline at $BASELINE_FILE" >&2 + exit 2 ;; +esac + +if [ "$found" -gt "$baseline" ]; then + echo "check-errexit-status-reads: $found status reads after an errexit decision, baseline is $baseline." >&2 + echo >&2 + printf '%s\n' "$listing" | sed 's/^/ /' >&2 + echo >&2 + echo "[bare] the shell dies at the assignment; the \$? line never runs." >&2 + echo "[decl] local/declare/export wins the status; \$? is ALWAYS 0." >&2 + echo "[source] bash 3.2 (macOS /bin/bash) dies here even with \`|| rc=\$?\`." >&2 + echo >&2 + echo "Lift errexit around it and restore it, as agmsg_terminal_load does:" >&2 + echo " case \$- in *e*) restore_e=1 ;; esac; set +e; x=\$(cmd); rc=\$?; [ \"\$restore_e\" = 1 ] && set -e" >&2 + exit 1 +fi + +if [ "$found" -lt "$baseline" ]; then + echo "check-errexit-status-reads: $found status reads after an errexit decision, below the baseline of $baseline." + echo "Lower the baseline in $BASELINE_FILE to $found so it cannot drift back up." + exit 1 +fi + +echo "check-errexit-status-reads: $found status reads after an errexit decision, at the baseline ($baseline)." diff --git a/.github/scripts/check-herdr-cli-routing.sh b/.github/scripts/check-herdr-cli-routing.sh new file mode 100755 index 000000000..e9ca6be8d --- /dev/null +++ b/.github/scripts/check-herdr-cli-routing.sh @@ -0,0 +1,95 @@ +#!/usr/bin/env bash +# +# Fail when scripts/drivers/terminals/herdr/ops.sh calls the `herdr` CLI directly +# from a function that is not on the named allowlist, or when an allowlisted +# function's count of direct calls moves in EITHER direction. +# +# WHY. A herdr pane id may be qualified by the socket of the instance that owns +# it (`:wN:pX`, #1055). Every call ABOUT that pane must reach that +# socket, which `_herdr_cli ...` does by setting HERDR_SOCKET_PATH from the +# id. A call that bypasses `_herdr_cli` raises no error: it goes to whatever +# instance the ambient environment names, and a pane id is only unique inside +# one instance -- so the call lands on a different seat's live pane (measured +# 2026-09-11, the accident #1055 is about). One missed site is silent, and a +# review that reads twenty-one one-line edits will miss one. So this is +# counted by machine. +# +# HOW IT COUNTS. By CALL POSITION, not by text match: a line is a direct call +# only when `herdr ` stands where a command stands -- at the start +# of a statement, or right after `$(`, `if`, `!`, `&&`, `||`, `|`, `then`, +# `else`, `do`, with any `VAR=value` assignment prefixes in between (a call +# that sets HERDR_SOCKET_PATH by hand is the bypass this exists to catch). +# Comment lines are skipped before matching, and `herdr:` inside a +# message string never has a subcommand word after it. (An earlier count of +# these calls by plain grep answered 55 where the true number is 34: comments +# and prose matched. That count is what this script must not repeat.) +# +# THE ALLOWLIST is `.github/herdr-cli-routing-allowlist`: one ` ` +# per line, naming the functions whose direct calls are instance-wide or +# deliberately ambient (listing every pane of an instance, spawning into the +# caller's own instance, describing the backend). A function not listed with a +# direct call fails; a listed function whose count went UP fails; a count that +# went DOWN fails too, and says to lower the entry -- an allowlist that is +# stale in the low direction lets the next addition hide inside the old number. +# `_herdr_cli` itself is the one function that must call `herdr` directly. + +set -u +root="$(cd "$(dirname "$0")/../.." && pwd)" +ops="$root/scripts/drivers/terminals/herdr/ops.sh" +allow="$root/.github/herdr-cli-routing-allowlist" +[ -f "$ops" ] || { echo "check-herdr-cli-routing: $ops not found" >&2; exit 2; } +[ -f "$allow" ] || { echo "check-herdr-cli-routing: $allow not found" >&2; exit 2; } + +# function -> number of direct `herdr ` calls at command position +counts="$(awk ' + /^[A-Za-z_][A-Za-z0-9_]*\(\)/ { fn=$0; sub(/\(\).*/,"",fn); next } + /^[[:space:]]*#/ { next } + fn != "" { + line=$0 + # strip a trailing comment that begins after whitespace + sub(/[[:space:]]#.*$/,"",line) + n=0 + # a command boundary, then optional spaces, then `herdr ` + # An assignment prefix (VAR=value ...) before the word is still a command + # position: `HERDR_SOCKET_PATH="$sock" herdr pane list` is exactly the form a + # bypass of _herdr_cli takes, and it must be counted, not hidden by the prefix. + while (match(line, /(^|\$\(|[;|&(]|[[:space:]](if|then|else|do|!)[[:space:]])[[:space:]]*([A-Za-z_][A-Za-z0-9_]*=("[^"]*"|[^[:space:]"]*)[[:space:]]+)*herdr[[:space:]]+[a-z][a-z-]*/)) { + n++ + line=substr(line, RSTART+RLENGTH) + } + if (n) c[fn]+=n + } + END { for (f in c) printf "%s %d\n", f, c[f] }' "$ops" | sort)" + +status=0 +# every function with direct calls must be listed with exactly that count +while read -r fn n; do + [ -n "$fn" ] || continue + want="$(awk -v f="$fn" '$1==f {print $2}' "$allow")" + if [ -z "$want" ]; then + printf ' %s calls herdr directly %s time(s) and is not on the allowlist: route it through _herdr_cli , or add "%s %s" to %s with a reason in the commit\n' "$fn" "$n" "$fn" "$n" "${allow#"$root/"}" + status=1 + elif [ "$n" -gt "$want" ]; then + printf ' %s: %s direct herdr calls, allowlist says %s -- ABOVE: a new direct call; route it through _herdr_cli or raise the entry deliberately\n' "$fn" "$n" "$want" + status=1 + elif [ "$n" -lt "$want" ]; then + printf ' %s: %s direct herdr calls, allowlist says %s -- BELOW: lower the entry, or the next addition hides inside the old number\n' "$fn" "$n" "$want" + status=1 + fi +done <<< "$counts" +# every listed function must still exist with a nonzero count +while read -r fn want; do + case "$fn" in ''|'#'*) continue ;; esac + if ! printf '%s\n' "$counts" | grep -q "^$fn "; then + printf ' %s is on the allowlist with %s but makes no direct herdr call now -- remove the entry\n' "$fn" "$want" + status=1 + fi +done < "$allow" + +total="$(printf '%s\n' "$counts" | awk '{s+=$2} END {print s+0}')" +if [ "$status" -eq 0 ]; then + echo "check-herdr-cli-routing: $total direct herdr calls, all on the allowlist at their listed counts." +else + echo "check-herdr-cli-routing: FAILED (direct herdr calls outside _herdr_cli must be listed by name and count)." +fi +exit "$status" diff --git a/.github/scripts/check-unguarded-env-reads.sh b/.github/scripts/check-unguarded-env-reads.sh new file mode 100755 index 000000000..af56ec0b9 --- /dev/null +++ b/.github/scripts/check-unguarded-env-reads.sh @@ -0,0 +1,142 @@ +#!/usr/bin/env bash +# +# Fail when `scripts/**/*.sh` grows a read of an ENVIRONMENT variable with no +# default, under shell options that make such a read fatal. +# +# WHY THIS IS STATIC, and not a test (#1129). Every entry point that reaches +# these files runs `set -euo pipefail` -- join.sh, actas-claim.sh, watch.sh, +# session-start.sh, inbox.sh and the rest. The bats suite does not: measured, +# `tests/test_helper.bash` sets no shell options at all, so a read that is only +# fatal under `-u` cannot fire inside a test. +# +# That was measured, not assumed. Forcing `set -u` into the shared test helper +# and running all 102 suites produced 8 reds, and NONE of them was a defect in +# `scripts/`: five were tests reading their own undefined variables, one was an +# artefact of where the option was placed, and two did not reproduce in +# isolation. Then the method was CALIBRATED against the one confirmed defect of +# this class -- #1126, `sock="${TMUX%%,*}"` with no default, which killed the +# tmux label search in a subshell whose caller discarded stderr. With `set -u` +# forced, `test_terminal_registry` on the tree that still contained it was +# 129/129 GREEN. The suite cannot see this class, so the guard has to be static. +# +# WHAT COUNTS AS A FINDING +# a read of an ALL-CAPS name that +# - is never assigned anywhere in the same file (so it comes from the +# environment or from a caller's export), and +# - is read without a default: `$NAME`, `${NAME}`, `${NAME%%,*}`, and +# - is not already guarded, in the same function, by an earlier +# `[ -n "${NAME:-}" ]` / `[ -z "${NAME:-}" ]`. +# +# The guard clause is what lets this go DOWN as well as up, and it was the +# difference between a calibrated check and a decorative one. Measured on three +# trees: +# +# 040a4c7 before #1112 introduced the defect 89 +# 07d76ef with the defect present 90 <- the new row is the defect +# b3fe77d after #1126 guarded it 89 +# +# Without the guard clause the count was 147 / 147 / 147: it would not have +# moved when the defect landed OR when it was fixed. +# +# TWO WAYS TO BURN ONE DOWN, and they are not equal. Giving the read a default +# (`${NAME:-}`) is the one that also makes the code correct wherever it runs. +# Adding a `[ -n "${NAME:-}" ]` guard in the same function is accepted here +# because it is what the tmux driver's own `terminal_detect` does and it makes +# an honest refusal -- but it only protects the reads BELOW it in that function. +# +# WHAT THIS DOES NOT SEE, said plainly so nobody reads a pass as a promise: +# - a variable assigned somewhere in the file and read before that line +# - a guard in a CALLER rather than in the same function +# - `${NAME:-}` used where a missing value is not actually acceptable +# The baseline is a COUNT, so it certifies nothing about the rows already in it. + +set -u + +ROOT="$(cd "$(dirname "$0")/../.." && pwd)" +# Overridable so the checker can be exercised against a fixture tree. A guard +# that can only be run against the real, already-clean tree has never been shown +# to fire. +BASELINE_FILE="${AGMSG_ENV_READS_BASELINE:-$ROOT/.github/unguarded-env-reads-baseline}" +SCAN_DIR="${1:-$ROOT/scripts}" + +count_and_list() { + python3 - "$1" <<'PY' +import re, sys, pathlib +root = pathlib.Path(sys.argv[1]) +# Op group: defaults (:- :+ :? / - + ?) plus assignment (:= / =). +# :- :+ :? skip this read; they are not assignments (#1197). +READ = re.compile(r'\$\{([A-Z][A-Z0-9_]*)(:[-+?=]|[-+?=])?[^}]*\}|\$([A-Z][A-Z0-9_]*)\b') +GUARD = re.compile(r'\[\s+-[nz]\s+"\$\{([A-Z][A-Z0-9_]*):-[^}]*\}"') +FUNC = re.compile(r'^([A-Za-z_][A-Za-z0-9_]*)\(\)\s*\{') +# Shell-provided or set-by-the-OS-everywhere: reading these unguarded is not the +# hazard this looks for. +SPECIAL = {'BASH_SOURCE','FUNCNAME','BASH_REMATCH','PIPESTATUS','OPTARG','OPTIND', + 'RANDOM','LINENO','SECONDS','BASHPID','BASH_VERSINFO','EUID','UID','PPID', + 'HOSTNAME','OSTYPE','MACHTYPE','SHLVL','REPLY','IFS','PATH','HOME','PWD', + 'TMPDIR','USER','SHELL','LANG','LC_ALL','TERM','COLUMNS','LINES','EDITOR'} +rows = [] +for f in sorted(root.rglob('*.sh')): + txt = f.read_text(errors='replace') + guarded = set() + for i, line in enumerate(txt.splitlines(), 1): + if FUNC.match(line): + guarded = set() + elif line == '}': + guarded = set() + s = line.strip() + if s.startswith('#'): + continue + for g in GUARD.finditer(line): + guarded.add(g.group(1)) + for m in READ.finditer(line): + name = m.group(1) or m.group(3) + op = m.group(2) + if not name or name in SPECIAL or op or name in guarded: + continue + # NAME= and NAME:= (the latter is ${NAME:=...} / ${NAME=...}). + if re.search(r'(?&2 + exit 2 +fi + +listing="$(count_and_list "$SCAN_DIR")" +if [ -z "$listing" ]; then + found=0 +else + found="$(printf '%s\n' "$listing" | wc -l | tr -d '[:space:]')" +fi + +baseline="$(tr -d '[:space:]' < "$BASELINE_FILE" 2>/dev/null || echo '')" +case "$baseline" in + ''|*[!0-9]*) + echo "check-unguarded-env-reads: no readable baseline at $BASELINE_FILE" >&2 + exit 2 ;; +esac + +if [ "$found" -gt "$baseline" ]; then + echo "check-unguarded-env-reads: $found unguarded environment reads, baseline is $baseline." >&2 + echo >&2 + printf '%s\n' "$listing" | sed 's/^/ /' >&2 + echo >&2 + echo "Every entry point that reaches these files runs 'set -euo pipefail', so a" >&2 + echo "read with no default kills the shell -- and inside a command substitution" >&2 + echo "whose caller discards stderr, it does so silently. Give the read a default" >&2 + echo "(\${NAME:-}), or guard the function with [ -n \"\${NAME:-}\" ] and refuse." >&2 + echo "The test suite cannot catch this: it runs with no shell options (#1129)." >&2 + exit 1 +fi + +echo "check-unguarded-env-reads: $found unguarded environment reads, at the baseline ($baseline)." diff --git a/.github/scripts/shard-tests.sh b/.github/scripts/shard-tests.sh index d6549fdf8..914a363b5 100755 --- a/.github/scripts/shard-tests.sh +++ b/.github/scripts/shard-tests.sh @@ -12,91 +12,61 @@ # the directory is assigned to exactly one shard, so the union of all shards is # always the whole suite (asserted by tests/test_ci_sharding.bats). # -# Balancing is by @test count, greedy longest-processing-time first, rather -# than by file count: the suite's files differ by more than an order of -# magnitude in size, so splitting on names alone would leave one shard doing -# most of the work and cap the speedup at whatever that shard costs. -# -# Test count is a proxy for runtime, and a loose one — measured on macOS, the -# whole suite is 860s and the count-balanced quarters (197/196/197/197 tests) -# come out at 125s/298s/366s/71s. Per-test cost varies from ~0.0s to ~8s -# depending on how much a file forks or waits. So the real speedup here is -# 860s -> 366s (~2.4x), not 4x. -# These are historical design measurements, not current weights. Every CI shard -# now uploads a bats-timings artifact with per-file and shard wall times; combine -# several with summarize-bats-timings.sh before changing the partition. -# -# It is still the right weight to ship first. The alternative, a checked-in -# table of measured per-file seconds, buys ~150s more but goes stale silently: -# it would be wrong the moment the fixed `sleep`s in the suite are replaced by -# condition polling, which is the very next CI change queued. Weights are worth -# revisiting once runtimes stop moving. Note the floor either way is the -# slowest single file (test_spawn.bats, 199s) — no split beats that, so the -# ceiling on this approach is ~4.5x, not 4x-and-then-some. -# -# Whatever the weights, the property that matters is coverage, not balance: the -# worst case of a bad weight is an unevenly filled shard, never a missing file. -# -# --- Pinned-apart files (#847, #848) --------------------------------------- -# -# @test count is a loose proxy for runtime in general (above), but for a -# specific shape of file it is not loose, it is blind: a file whose cost is -# almost entirely waiting (background processes, poll loops with -# hundred-plus-iteration bounds) rather than how many @test blocks it -# contains can carry a tiny weight here while dominating its shard's actual -# wall clock. A file that is merely large -- many @test blocks, ordinary -# per-test cost -- is NOT this case; count already weights it correctly, and -# it is not pinned. -# -# First ranked 2026-08-19 by correlating each `ok N ` line's own GitHub -# Actions timestamp against which file's `@test` block that description belongs -# to, then ranking every file by seconds-per-test rather than by raw duration -# (raw duration alone does not distinguish "slow because few tests wait a long -# time" from "slow because there are simply many tests", and only the former is -# what count weighting misses). Re-measured 2026-09-12 from the per-file wall -# times that run-bats-timed.sh now records (#1159), read off the green run that -# first carried them (GitHub Actions run 34664794167, tests.yml bats job, -# macos-latest, 5 shards): -# -# tests/test_remote_engine_start_refusal.bats 679s / 9 tests = ~75s/test -# tests/test_remote_status_liveness.bats 293s / 31 tests = ~9s/test -# -# against a whole-suite per-test cost this script's own header already says -# runs ~0.0s-8s. Both are 1.5x-10x above that ceiling on a low test count, so -# both rank near the bottom of the count-weighted sort while carrying some of -# the largest absolute durations in the suite. (Files that are merely large in -# absolute terms -- e.g. a 179-test file at a very ordinary ~1.2s/test -- were -# checked and excluded: their weight already reflects their real cost.) -# -# #847's own trigger was exactly this class of file landing next to another -# heavy one purely because an unrelated 15-test addition elsewhere repacked -# the partition — the count weight cannot tell "heavy because slow" from -# "heavy because voluminous", so nothing stops two slow-but-few-tests files -# from drifting onto the same shard as the tree changes shape. Pinning these -# apart, in fixed shard slots decided before the ordinary weighted pass runs, -# means no future change to any OTHER file's test count can put two of them -# together again — that was the actual, demonstrated failure, not merely a -# theoretical one. -# -# This does not bound a shard's total duration: the heavier entry above -# (679s) is heavy enough on its own that no repacking of the rest of the -# suite moves its shard's floor by much. See tests.yml's bats-shard -# timeout-minutes for the ceiling this is paired with, sized to cover that -# floor plus a fair share of everything else with real margin. And a file NOT -# on this list can still turn out to be similarly disproportionate and land -# next to another one by chance — nothing here detects that case in general, -# only these two measured instances of it. Revisit alongside the counting -# scheme itself once the concurrent effort to shorten these files (tracked -# separately from #847/#848) lands and the numbers above are stale. -# -# FOLLOW-UP: whichever of that effort's PRs (#876 et al.) touches either file -# named below changes its real cost, possibly enough to make pinning it -# pointless or to make some other, currently-unremarkable file the next -# hidden outlier. Re-run this script's own measurement method (correlate a -# green run's `ok N` timestamps against each file, rank by seconds-per-test) -# on main once that work lands, and drop or replace entries here based on -# what it says then — this list is not meant to be permanent. The 30-minute -# job cap in tests.yml is a separate decision and does not depend on this one. +# Balancing is by each file's real measured wall time, greedy +# longest-processing-time first, rather than by file count or @test count: +# the suite's files differ by more than an order of magnitude in cost, so +# splitting on names alone would leave one shard doing most of the work and +# cap the speedup at whatever that shard costs, and @test count is only a +# loose proxy for that cost (a file whose time is almost entirely waiting -- +# background processes, poll loops -- can carry a tiny count while +# dominating its shard's wall clock; see #847/#1243 below for the measured +# instance of this that motivated moving off count entirely). +# +# Real time comes from .github/scripts/bats-file-seconds.tsv, a checked-in +# table of per-file macOS wall seconds measured from a real CI run (see that +# file's own header for which run and how to refresh it). A file with no row +# in the table -- renamed, or added since the last measurement -- is +# estimated at the table's own average seconds/file, never at zero: coverage +# (every file lands in exactly one shard) does not depend on the table being +# current, only balance does, and an unmeasured file at the average is a far +# better guess than an unmeasured file at zero. +# +# Whatever the weights, the property that matters MOST is coverage, not +# balance: the worst case of a bad weight is an unevenly filled shard, never +# a missing file. +# +# --- Pinned-apart files (#847, #848, #1243) --------------------------------- +# +# Two files are additionally seeded into FIXED, distinct shard slots before +# the ordinary weighted pass runs, rather than simply sorting into place by +# their (now real-time) weight like everything else: +# +# test_remote_engine_start_refusal.bats +# test_remote_status_liveness.bats +# +# Real-time weighting already prevents either from dominating its shard's +# wall clock unnoticed -- that was #847's original problem, and it is what +# @test-count weighting could not see. What fixed-slot seeding buys ON TOP of +# that is a STRUCTURAL guarantee, independent of whatever else the tree looks +# like on a given day: these specific two files can never land on the same +# shard, at any shard total >= 2 (asserted by tests/test_ci_sharding.bats). +# Without it, two files that happen to be the two heaviest overall could in +# principle still be placed together by an unlucky greedy pass at a low +# shard total -- fixed slots rule that out by construction rather than by +# probability. +# +# This seeding does NOT exclude these two shards from the rest of the +# weighted pass (#1243 revision): earlier this pinned a shard by excluding it +# outright, which produced a *correctness* guarantee (never refilled) at the +# cost of *balance* -- on the merged (2550 @test) tree, forcing all +# non-pinned work onto only 3 of 5 shards floored the worst case at ~29 +# minutes of the 30-minute cap, regardless of how well those 3 were balanced +# (5243s of non-pinned macOS work over 3 shards = ~1748s = 29.1min each, even +# under perfect packing). Letting the other files fill a pinned shard's +# remaining real-time budget -- exactly like any other shard -- brings every +# shard to ~20.8 minutes on that same measured tree, because the two pinned +# files' own real costs (660s, 346s) are ordinary-sized next to the whole +# suite's 6249s once every file is weighted in the same unit. # # Matched by basename, not by the `$dir`-relative path `files` below uses, so # the pin still resolves when this script is invoked against a different @@ -132,58 +102,56 @@ case "$total" in ''|*[!0-9]*) usage ;; esac files="$(find "$dir" -maxdepth 1 -name '*.bats' | LC_ALL=C sort)" [ -n "$files" ] || { echo "${0##*/}: no .bats files under $dir" >&2; exit 1; } -# Weight each file by its number of test cases. `grep -c` exits 1 on no match -# after printing 0, which set -e would otherwise treat as fatal. -file_weight() { - local n - n="$(grep -c '^[[:space:]]*@test' "$1" || true)" - [ -n "$n" ] || n=0 - printf '%s' "$n" -} +# The checked-in table of measured per-file macOS seconds this script weights +# by. Overridable so this script's own tests can point at a fixture table +# without touching the real one. +SECONDS_TABLE="${SHARD_TESTS_SECONDS_TABLE:-$(CDPATH= cd -- "$(dirname -- "$0")" && pwd)/bats-file-seconds.tsv}" -# Load a pinned file contributes when it is seeded, in the same unit the -# weighted pass below uses: @test-equivalents. A pinned file's @test count -# (its file_weight) badly understates its cost -- that is the whole reason it -# is pinned -- so seeding it at that count leaves its shard looking almost -# empty and the greedy pass piles an average share of other files on top of an -# already-expensive file. Seed it instead at what its measured wall time is -# WORTH in average tests: round(measured_macOS_wall_seconds / avg_s_per_test). -# -# This is tied to the file's OWN measured cost, not to a count the suite -# outgrows, so it does not drift as the suite grows -- which is exactly the -# failure #1107 was (a partition sized against a count baseline that went -# stale as the suite roughly tripled). -# -# MEASURED AGAINST: GitHub Actions run 34664794167 (#1159, tests.yml bats -# job, macos-latest, 5 shards, 2026-09-12). Whole-suite macOS wall summed -# over every file = 4212s across 1749 @test blocks => avg = 4212 / 1749 = -# 2.408 s/test. Both seeds below are that run's per-file wall / that avg: -# test_remote_engine_start_refusal.bats 679s / 2.408 = round(281.9) = 282 -# test_remote_status_liveness.bats 293s / 2.408 = round(121.7) = 122 -# -# The numerator (per-file wall) and the divisor (whole-suite avg) came from the -# SAME run, so a refresh must re-derive BOTH together off one green run's -# bats-timings artifacts (summarize-bats-timings.sh) -- never update one alone. -# A pinned file with no measured seed here falls back to its @test count. -# Exact string equality, not `case`: a bare `case` glob widens under an -# inherited `nocasematch`/locale, and these are meant to match one literal -# basename each and nothing else. -pin_seed() { - if [ "$1" = test_remote_engine_start_refusal.bats ]; then - printf 282 - elif [ "$1" = test_remote_status_liveness.bats ]; then - printf 122 - else - file_weight "$2" +# The table's own average seconds/file, used below as every unmeasured file's +# estimate. Computed from the table itself (not a second hardcoded number) +# so it can never silently drift out of step with the table it is meant to +# summarize; nearest-integer rounding via the usual awk `(x+0.5)` idiom. +# Falls back to a small positive constant if the table is missing or empty +# so an unmeasured file is still weighted something, never zero or an error. +avg_seconds=1 +if [ -r "$SECONDS_TABLE" ]; then + avg_seconds="$(awk -F'\t' ' + $0 !~ /^#/ && NF == 2 { sum += $2; n++ } + END { if (n > 0) printf "%d", (sum / n) + 0.5; else print 1 } + ' "$SECONDS_TABLE")" + [ -n "$avg_seconds" ] || avg_seconds=1 +fi + +# Weight a file by its own measured real seconds from the table, matched by +# basename (see the header comment above on why basename, not path). A file +# absent from the table -- renamed, or added since the measurement -- is +# estimated at the table's average rather than treated as zero-cost or an +# error: this function must never fail or omit a value, since every file in +# the tree passes through it once and a missing weight would silently drop +# that file from the balance pass (not from coverage, which does not use +# this value at all). +file_seconds() { + local base line + base="$(basename "$1")" + if [ -r "$SECONDS_TABLE" ]; then + line="$(awk -F'\t' -v b="$base" '$1 == b { print $2; exit }' "$SECONDS_TABLE" 2>/dev/null || true)" + if [ -n "$line" ]; then + printf '%s' "$line" + return + fi fi + printf '%s' "$avg_seconds" } # Seed the pinned files into distinct shards first, in PINNED_APART's own -# (measured-heaviest-first) order — not the order they happen to sort in -# below, which is by count and is exactly the metric these files defeat. Each -# consumes one shard slot (wrapping if there are more pinned files than -# shards); everything else is decided by the ordinary weighted pass afterward, -# which never reconsiders a file placed here. +# order (each consumes one shard slot, wrapping if there are more pinned +# files than shards) -- not sorted into place by weight like everything +# else, because their whole point is a STRUCTURAL "never share a shard" +# guarantee (see the header comment), which sorting cannot provide even +# though real-time weighting alone now keeps either from dominating its +# shard unnoticed. `load` starts seeded with each pinned file's own real +# cost; the ordinary weighted pass below is free to add more to these same +# shards; it does not skip them. i=0 while [ "$i" -lt "$total" ]; do load[i]=0 @@ -205,7 +173,7 @@ $files EOF [ -n "$match" ] || continue s=$((slot % total)) - load[s]=$((load[s] + $(pin_seed "$p" "$match"))) + load[s]=$((load[s] + $(file_seconds "$match"))) if [ "$s" -eq "$((index - 1))" ]; then printf '%s\n' "$match" fi @@ -213,14 +181,14 @@ EOF slot=$((slot + 1)) done -# Weight every remaining (non-pinned) file the same way as before. +# Weight every remaining (non-pinned) file by its own real seconds. weighted="" while IFS= read -r f; do [ -n "$f" ] || continue case "$pinned_paths" in *" $f "*) continue ;; esac - weighted="${weighted}$(file_weight "$f") ${f} + weighted="${weighted}$(file_seconds "$f") ${f} " done < 5 (#1107). Same change already on the terminal-driver integration branch; - # this is the main-targeted twin so main PRs stop hitting the same macOS cap. - # shard-tests.sh seeds each pinned file at one shard's AVERAGE share - # (total_tests/total), which grows with the suite while the pinned files do not, - # over-reserving the two pin shards and pushing the real load onto the non-pin - # ones. main is 1742 @tests here, so the worst-case non-pin shard - # (total*(N+2)/N^2) is 653 at N=4 and 488 at N=5 -- and a 4-shard macOS leg was - # already reaching the 30-min cap while still passing tests (a slow shard, not a - # hang: the workflow's own #848/#857 note). Five, not six, because the account - # runs at most FIVE macOS jobs at once (the concurrency header): a sixth would - # queue behind the other five of its own run. This does NOT fix the scaling - # property (pin_seed still grows, count still under-values wait-dominant files); - # that re-fit is #847. + # 4 -> 5 (#1107). The suite outgrew a 4-shard partition and a 4-shard macOS + # leg was reaching the 30-min cap while still passing tests (a slow shard, + # not a hang). Five, not six, because the account runs at most FIVE macOS + # jobs at once (the concurrency header): a sixth would queue behind the + # other five of its own run, inflating the wall clock past what a lighter + # shard buys. + # + # shard-tests.sh weights every file (pinned or not) by its own real measured + # macOS seconds from .github/scripts/bats-file-seconds.tsv (#1243) rather + # than @test count or a count-equivalent fit -- count was never more than a + # loose proxy for a file's real cost, and two prior fits against it (#847, + # then a #1243 predecessor that excluded pinned shards outright) each + # eventually drifted or traded balance away as the suite grew. Re-measure + # that table (its own header says how) when the suite's shape has moved + # enough that tests/test_ci_sharding.bats's balance-ratio assertion goes + # red, the same way #1107 itself was tracked: a constant sized against an + # older system stays right only as long as the system does. SHARD_TOTAL: 5 # The Windows legs' sqlite3, pinned three ways (#824). # @@ -706,6 +710,18 @@ jobs: # differently there than on the hosts that were measured. The engine's # own bats leg is ubuntu and macos only, so this selects the two tests # that would notice and runs them on the platform in question. + # The codex hook wrapper (#1015). The defect is a Windows-only path -- a + # PowerShell one-liner that invokes Git Bash as a LOGIN shell, so the + # profile's output lands on the stdout codex parses -- and the fix + # moves the payload off that stdout. The bash half is bash's + # everywhere and is proved on the POSIX legs; the PowerShell half is + # asserted as a string there because nothing on those hosts can run + # it. This leg is the only place it is EXECUTED. Without it the fix + # ships to Windows having never run on Windows. + - leg: codex hook wrapper (#1015) + filter: "windows-wrapper" + target: tests/test_delivery.bats + sqlite: true - leg: driver input (#817) filter: "driver-input" target: tests/test_remote_sync_driver_input.bats @@ -1140,6 +1156,44 @@ jobs: - name: No new assertion that cannot fail run: .github/scripts/check-enforced-assertions.sh + errexit-status-reads: + name: errexit status reads + runs-on: ubuntu-latest + timeout-minutes: 5 + # Unconditional for the same reason as `enforced-assertions` above: a job + # that skips on some diffs is a required context that can sit pending. It + # is a static read of `scripts/**/*.sh`. + steps: + - uses: actions/checkout@v4 + + # Ubuntu's bash is 5.x, and one of the three shapes this looks for is + # fatal ONLY on bash 3.2 (macOS /bin/bash). That is why the check is + # static: running it under one interpreter would miss the shape that + # kills the other. The behaviour was measured on both (see the header); + # what runs here is the count. + - name: No status read that errexit already decided + run: .github/scripts/check-errexit-status-reads.sh + + unguarded-env-reads: + name: unguarded env reads + runs-on: ubuntu-latest + timeout-minutes: 5 + # Unconditional, like its two siblings above: a job that skips on some + # diffs is a required context that can sit pending. It is a static read of + # `scripts/**/*.sh`. + steps: + - uses: actions/checkout@v4 + + # The suite cannot do this one. Every entry point that reaches those + # files runs `set -euo pipefail`; tests/test_helper.bash sets no shell + # options, so a read that is only fatal under `-u` never fires in a test. + # Measured (#1129): with `set -u` forced into the helper, all 102 suites + # produced 8 reds and none was a defect in scripts/ -- and the suite was + # 129/129 green on the tree that still held the one confirmed defect of + # this class (#1126). + - name: No unguarded environment read + run: .github/scripts/check-unguarded-env-reads.sh + private-names: name: internal names runs-on: ubuntu-latest diff --git a/.gitignore b/.gitignore index 8745b97f1..d8e891f2e 100644 --- a/.gitignore +++ b/.gitignore @@ -14,3 +14,4 @@ teams/ .env .env.* /.secrets/ +run/ diff --git a/CHANGELOG.md b/CHANGELOG.md index fffd5eaba..ebe71ffa6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -456,7 +456,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Support spawning into herdr panes (#495) - Drag files onto a pane to insert their path (#481) - Adaptive catch-up so a backlog doesn't crawl at 100/5s -- Add team-list.sh (agmsg team list --json, koit-approved) +- Add team-list.sh (agmsg team list --json) - Add status --json and pending list/abort (ADR 0007 addendum) - Consume connected team credentials - Add scripts/remote.sh (connect/status/disconnect/doctor) per ADR 0007 @@ -491,10 +491,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Resolve symlinks before trampoline compare; doctor checks node - Detect the macOS CLT python3 trampoline, not just PATH presence - Close the python3 dependency-tiering gap on the remote path -- Close the integer-overflow bypass in AGMSG_TEAM_LIST_MAX_TEAMS validation (co1 delta review round 2) -- Validate AGMSG_TEAM_LIST_MAX_TEAMS as a positive integer (co1 delta review) +- Close the integer-overflow bypass in AGMSG_TEAM_LIST_MAX_TEAMS validation (delta review round 2) +- Validate AGMSG_TEAM_LIST_MAX_TEAMS as a positive integer (delta review) - Fail closed on incompleteness; shrink v1 schema -- Wire 'agmsg team list' into actual dispatch entry points (co1 P1) +- Wire 'agmsg team list' into actual dispatch entry points (review P1) - Stop binding config JSON via .param set (#87-class tokenizer bug) - Hide imported identity at TTY - Separate token input from E2EE prompts @@ -529,15 +529,15 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Read messages from the event log too, not just the legacy table - Assert against the event log, not the legacy messages table - Escape interpolated names in rename/rename-team SQL (#223, #87) -- Jsonl compact keys reads by tuple, not a space-join (co1 #221) +- Jsonl compact keys reads by tuple, not a space-join (#221 review) - Make the jsonl driver parse under macOS bash 3.2 (#207, #221 CI) -- Jsonl mark aborts on a failed existing-reads scan (co1 #207 residual) -- Jsonl driver must not swallow failures as ok (co1 #207 review) +- Jsonl mark aborts on a failed existing-reads scan (#207 residual) +- Jsonl driver must not swallow failures as ok (#207 review) - Watch-once stale-wake token = unread-set digest, not a max id (#207) -- Document --limit semantics + make storage_history agent truly optional (co1 #206 review) -- Export skips unknown event types; pin high-water with a tail-duplicate test (co1 #205 review) -- Describe is a metadata op; surface backend errors; chronological reads (co1 re-review, #204) -- Legacy read, pipefail framing, §1.4 control ops (co1 review, #204) +- Document --limit semantics + make storage_history agent truly optional (#206 review) +- Export skips unknown event types; pin high-water with a tail-duplicate test (#205 review) +- Describe is a metadata op; surface backend errors; chronological reads (re-review, #204) +- Legacy read, pipefail framing, §1.4 control ops (review, #204) ### Performance - Seal a bulk push page in parallel (#502) @@ -565,8 +565,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Close Stage 2 frontier edge cases - Define Stage 2 read-state synchronization - Note rename.sh/rename-team.sh/api.sh as sqlite-coupled known gaps -- Correct the ctrl:despawn cursor-advance comment (co1 step-3 review) -- Clarify stdout framing, cursor token, watch tip (co1 review, #203) +- Correct the ctrl:despawn cursor-advance comment (step-3 review) +- Clarify stdout framing, cursor token, watch tip (review, #203) - Storage contract §2 — messages-only, opaque cursor, recipient-scoped read (#203) - Draft ADR 0003 — storage axis driver ABI, contract, scope (proposed) @@ -794,7 +794,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Documentation - Add supported-agents logo strip -- List hermes in the --agent-type help (co1 nit) +- List hermes in the --agent-type help (review nit) - Add docs/plugins.md + README section + plugins/ drop-in dir - Refresh manifest table + paths for the 1.1.0 layout - Lead Quick Start with npx, the zero-clone install path diff --git a/README.md b/README.md index b775aedf9..aed9141d1 100644 --- a/README.md +++ b/README.md @@ -334,13 +334,20 @@ See [docs/opencode.md](docs/opencode.md) for full setup instructions. ~/.agents/skills//scripts/send.sh "" [--force] ~/.agents/skills//scripts/inbox.sh ~/.agents/skills//scripts/history.sh [agent_id] [limit] -~/.agents/skills//scripts/team.sh +~/.agents/skills//scripts/team.sh [--json | --fix | --fix-pane-names | --rename-sessions] +~/.agents/skills//scripts/placement-collisions.sh ~/.agents/skills//scripts/whoami.sh ~/.agents/skills//scripts/delivery.sh set ~/.agents/skills//scripts/delivery.sh status [ ] ~/.agents/skills//scripts/reset.sh [agent_id] ``` +`team.sh` combines the roster with terminal placement, activity, delivery mode, and identity consistency. Verified identity is collapsed to `identity=ok`; mismatches and values that could not be observed are expanded with their evidence. `--json` emits every field for every registration. The repair flags report each action per identity cell as `changed`, `skipped`, `failed`, or (for a session name that could not be read back) `poked_unverified`. They are two different kinds of act: `--fix-pane-names` repairs the pane label and agent key through the terminal's own API and never types into a session; `--rename-sessions` repairs the CLI session name by typing the type's rename command (`/rename -` for Claude Code, whatever the type's manifest declares otherwise) into the pane, and only after positively identifying a ready process there. `--fix` does both, unconditionally, including the keystroke. Pane liveness will join this view when the pane-state contract lands; until then the unavailable column is omitted rather than filled with `unknown`. + +`placement-collisions.sh` is a separate, read-only installation-wide report. It never repairs or removes a record; keeping this fleet observation outside `team.sh` prevents an operator-level scan from becoming part of a seat's repair path. Today it reports only the record-only layer: two DIFFERENT seats' records resolved to the same canonical (kind, instance, pane) locator, entirely from records on disk — no terminal is ever asked anything. A ref with no instance component to resolve (every herdr ref today; a legacy bare tmux `%N`/`@N`) is not joined by raw string equality; it is listed under `unscoped_records` instead, since record-only evidence cannot tell such refs apart across terminal instances. `collisions: none` means the walk completed and found nothing; `collisions: none_observed` paired with `coverage: partial` means something along the way (a team config, a placement record, an empty ref) could not be read, so the empty answer is not a proven one; `collisions: not_attempted` means there was no `teams/` directory to walk at all. An actual-location layer — matching a seat's own record against where a live census actually observes it — is designed but not yet wired here; see the script's header. + +Terminal identity has a different number of observable names on each backend. Herdr exposes three independent values: the visible pane label, its internal agent key, and the CLI session name. tmux exposes two: the `@agmsg_agent` pane option is the internal key, while the CLI owns `pane_title`, so there is no independent pane-label field after the CLI starts. `team.sh` reports that tmux field as `n/a` rather than treating an unavailable concept as a mismatch. + `send.sh` takes four positional arguments — ` ""` — plus an optional trailing `--force`. Quote the message so the shell sees it as one argument; an unquoted message with spaces will be misparsed. Both `from` and `to` must already be registered in ``; an unregistered name errors out (listing the currently registered names) instead of silently storing an undeliverable message. Pass `--force` to bypass this check for an intentional pre-registration send. ## FAQ / Design notes diff --git a/SKILL.md b/SKILL.md index 57d420ec8..8c6b85a3e 100644 --- a/SKILL.md +++ b/SKILL.md @@ -1,230 +1,230 @@ --- -name: agmsg -description: Cross-agent messaging via SQLite. Send messages between Claude Code, Codex, Gemini CLI, GitHub Copilot CLI, and other agents. No daemon, no network, no dependencies beyond bash and sqlite3. +name: __SKILL_NAME__ +description: Cross-agent messaging via SQLite. Send messages between Claude Code, Codex, Gemini CLI, and other agents. No daemon, no network, no dependencies beyond bash and sqlite3. --- -# Agent Messaging + -**IMPORTANT: Always use the provided scripts. NEVER directly read or edit config files, DB, or team data. There is NO register.sh — use join.sh to join a team.** +Agent messaging command. **IMPORTANT: Always use the provided scripts. NEVER directly read or edit config files, DB, or team data. There is NO register.sh — use join.sh to join a team.** -**Shell requirement:** All agmsg scripts are Bash scripts. Always execute them via `bash`, never via PowerShell or cmd directly. If your default shell is not Bash (e.g. PowerShell on Windows), wrap every command with `bash -lc '...'`. Example: `bash -lc '~/.agents/skills/agmsg/scripts/send.sh myteam alice bob "hello"'`. Do NOT construct DB paths manually — the scripts handle path resolution internally. If you need to redirect storage, use `AGMSG_STORAGE_PATH` (the supported override). +**Use agmsg, not the host agent's own inter-session messaging.** Several agent +CLIs ship a native way for one session to message another on the same machine +(in Claude Code, the `SendMessage` / `ListAgents` tools over its peer-session +list). While a project is on agmsg, route agent-to-agent messages through agmsg +instead. A message sent natively does not exist as far as agmsg is concerned: +it is absent from `history.sh` and the team's export, it never reaches a member +on another machine through remote sync, it does not mark read or advance any +cursor, and it cannot address a member whose CLI is a different type. Half the +conversation living somewhere unrecorded is worse than either channel alone, +and the gap is invisible until someone reads the history and finds a decision +with no message behind it. The native channel stays fine for anything outside +the team — a subagent you spawned for your own task, or a session that has not +joined. -## How to use +**Shell requirement:** All agmsg scripts are Bash scripts. Always execute them via `bash`, never via PowerShell or cmd directly. If your default shell is not Bash (e.g. PowerShell on Windows), wrap every command with `bash -lc '...'`. Example: `bash -lc '~/.agents/skills/__SKILL_NAME__/scripts/send.sh myteam alice bob "hello"'`. Do NOT construct DB paths manually — the scripts handle path resolution internally. If you need to redirect storage, use `AGMSG_STORAGE_PATH` (the supported override). -### Step 0: First-run bootstrap + + -agmsg keeps its SQLite database, team registry, and runtime state under `~/.agents/skills/agmsg/`. The `./install.sh` install path creates that tree; the Claude Code plugin install path does not (the plugin marketplace flow only drops the skill content into `~/.claude/plugins/cache/`). Before any other command, bootstrap if needed: +## Identity -```bash -if [ ! -d ~/.agents/skills/agmsg ]; then - # Locate the plugin install script (any version), run it once. - installer=$(ls ~/.claude/plugins/cache/fujibee-agmsg/agmsg/*/install.sh 2>/dev/null | head -1) - if [ -n "$installer" ]; then - bash "$installer" --cmd agmsg - else - echo "agmsg not installed. Either:" >&2 - echo " - run ./install.sh in the agmsg repo, or" >&2 - echo " - install via /plugin marketplace add fujibee/agmsg && /plugin install agmsg@fujibee-agmsg" >&2 - exit 1 - fi -fi -``` - -After this runs once, `~/.agents/skills/agmsg/` is populated and you can skip Step 0 on future invocations. - -### Step 1: Check identity - -```bash -~/.agents/skills/agmsg/scripts/whoami.sh "$(pwd)" -# type: claude-code, codex, gemini, antigravity, copilot -# Returns: agent=... / multiple=true ... / suggest=true ... / not_joined=true ... -``` - -### Step 2a: If not in a team — join one - -Before first-time setup, inspect the user's request. If they ask to join, -import, or bring in a team that already exists on a server, do not run -`join.sh`. Go directly to the `remote pull` command under Step 2b. Before -pulling, run `team-list.sh --json --scope all`; if a same-named local team has -`binding_state` `none` or `disconnected`, stop and ask the user how to proceed. -After pull succeeds, return here so the user can register a new local agent in -the team that pull just created. - -Ask the user for a team name. If it's an existing team, run `team.sh ` first to see the current roster and note the names already in use. Look for a naming convention already in play (e.g. a shared base name with role and number suffixes (`-`), or names derived from the team name) and, when one exists, propose 2-3 unused names that extend it; otherwise propose 2-3 short, distinctive identity names (not a bare tool-type label like `codex`/`cc`). Either way, names must not collide with the roster. For a brand-new team, skip the roster check and just ask. Then run: - -```bash -~/.agents/skills/agmsg/scripts/join.sh "$(pwd)" [--force] -``` - -Do NOT manually edit config files. Always use join.sh. If the name was recently renamed away with `rename.sh`, join.sh refuses to revive it (printing the new name it maps to) instead of silently re-registering it — this guards against a CLI slash-command history resubmitting `actas ` after a rename. Pass `--force` only for a deliberate, unrelated reuse of that exact name. - -### Step 2b: If already in a team — execute command - -**Default (no arguments): IMMEDIATELY check inbox. Do NOT ask what to do.** - -```bash -# Check inbox (marks messages as read) — DEFAULT action -~/.agents/skills/agmsg/scripts/inbox.sh - -# Send a message (from/to must already be registered in ; add --force to bypass) -~/.agents/skills/agmsg/scripts/send.sh "" [--force] - -# Message history -~/.agents/skills/agmsg/scripts/history.sh [agent_id] [limit] - -# Export a team's message history as JSONL — one message_sent record per line, -# chronological. Default to stdout (pipeable); --out writes a file. -# --agent limits to one agent; --limit keeps the most recent N (omit = all -# currently retained). Output is plaintext (the local store is plaintext). -~/.agents/skills/agmsg/scripts/export.sh --team [--agent ] [--limit N] [--out ] - -# List team members -~/.agents/skills/agmsg/scripts/team.sh - -# List every locally known team (read-only, secret-free — "agmsg team list"). -# Distinct from `team.sh ` above: check for "team list" FIRST so -# "list" is never mistaken for a team name. --json emits a strict, -# versioned object ({schema_version, teams: [{name, remote_team_id, scope, -# binding_state}]}) and exits non-zero with NO payload if any team was -# unreadable or the count was truncated — never a partial list dressed up -# as complete. See scripts/team-list.sh's own header comment for the exact -# enums and why onboarding_state/promote_eligible/blocked_reason are -# deliberately NOT in this schema yet (their meaning depends on ADR 0010, -# which hasn't landed). -~/.agents/skills/agmsg/scripts/team-list.sh [--json] [--scope all|project] [] - -# Leave a team -~/.agents/skills/agmsg/scripts/leave.sh - -# Rename a team (moves dir, updates config + messages). -# After renaming, each existing member should re-run whoami.sh to refresh -# their cached team name in any running session. -~/.agents/skills/agmsg/scripts/rename-team.sh - -# Show the installed version — the git-describe provenance string recorded at -# install time (tag + commits-since + abbreviated commit, plus -dirty when -# installed from a tree with uncommitted changes). See #117. -~/.agents/skills/agmsg/scripts/version.sh - -# Clear registrations for the current project/type. -# A trailing additionally releases any actas exclusivity locks -# this session held on so peers can pick them up immediately. -~/.agents/skills/agmsg/scripts/reset.sh "$(pwd)" [agent_id] [session_id] - -# Set delivery mode for this project. -# monitor — real-time push via SessionStart + Monitor tool (claude-code only) -# turn — Stop-hook pulls at the end of each assistant turn -# both — monitor primary, turn as fallback -# off — no automatic delivery -~/.agents/skills/agmsg/scripts/delivery.sh set "$(pwd)" -~/.agents/skills/agmsg/scripts/delivery.sh status "$(pwd)" - -# Multiple roles per project (one CC = one active role). -# Claude Code: `actas` claims an exclusivity lock for across sessions -# and restarts the Monitor filtered to only; peer watchers stop -# subscribing to while this session holds the lock. `drop` releases. -# Codex: actas is send-side only (no stable session_id during slash commands -# → no peer-visible lock). See README "Codex caveat" for details. -# If is new and none was given upfront (bare `actas`, or the user asks -# for a suggestion), check the target team's roster first (team.sh ). -# Look for a naming convention already in play (e.g. a shared base name with -# role and number suffixes (-), or names derived from the -# team name) and, when one exists, propose 2-3 unused names that extend it; -# otherwise propose 2-3 short, distinctive names. Either way, names must not -# collide with the roster. Ask the user to pick before continuing. -~/.agents/skills/agmsg/scripts/actas-claim.sh "$(pwd)" "$session_id" -~/.agents/skills/agmsg/scripts/reset.sh "$(pwd)" "$session_id" - -# (Both of the above are normally driven by `/agmsg actas ` and -# `/agmsg drop ` slash commands, which also handle the Monitor -# TaskStop + relaunch dance described in the cmd template.) - -# Spawn a NEW agent process that takes an actas identity on boot. -# Pre-joins to a team, then launches the agent CLI in a tmux pane/window -# (when run inside tmux) or a new OS terminal, with `/agmsg actas ` as the -# initial prompt. By default it BLOCKS until the new agent's watcher attaches -# (prints `status=ready`), so a leader can send work right after spawn returns -# without losing it to the agent's cold start. claude-code/codex only; macOS -# primary, Linux/Windows best-effort. Non-tmux + no usable terminal (headless) -# errors out. -# --project project to launch in (default: $PWD) -# --team team to join into (default: auto-resolved from project) -# --window new tmux window instead of splitting the current one -# --split h|v tmux split direction (default h) -# --terminal terminal command template ({cmd} = path to the boot -# script) for the non-tmux path; overrides $AGMSG_TERMINAL -# / config spawn.terminal. macOS default uses `open -a` -# (no Automation/TCC permission prompt). -# --no-wait don't block on readiness (fire-and-forget) -# --ready-timeout N seconds to wait for readiness (default 90; on timeout -# prints status=timeout and exits 3). Codex skips the -# wait (it has no Monitor). -# --boot-prompt hand the new agent an initial task: the boot prompt -# becomes the actas command followed (newline-separated) -# by , so it claims its identity AND starts the task -# in its first turn. The only way to give a one-shot goal -# to a codex peer (no Monitor → a post-spawn send to its -# idle session is never noticed). -~/.agents/skills/agmsg/scripts/spawn.sh [options] - -# Tear down a spawned member — the inverse of spawn. -# Default (graceful): sends a `ctrl:despawn` control message to ; the -# member's watcher drops its own role (releasing the actas lock + registration) -# and closes its own tmux pane, ending the agent. Blocks until the lock releases -# (--timeout, default 30s) then prints `status=ok`; on timeout prints -# status=timeout and exits 3 (retry with --force). Only an exclusive watcher -# dedicated to acts on it — the despawning session is never torn down. -# --force: skip the message and tear the member down from the placement recorded -# at spawn time (kill its tmux pane/window, drop its registration) — for a dead -# watcher or a codex member (no Monitor). A hand-started member with no placement -# record can't be --forced. -# --force tear down from the recorded placement, no message -# --timeout N seconds to wait for graceful teardown (default 30) -~/.agents/skills/agmsg/scripts/despawn.sh [--force] [--timeout N] -``` - -### Rename +If you already know your AGENT and TEAMS from a previous `__CMD_PREFIX____SKILL_NAME__` call in this session, skip to **Execute** below. + +Otherwise, run: `~/.agents/skills/__SKILL_NAME__/scripts/whoami.sh "$(pwd)" __AGENT_TYPE__` + +Four possible outputs: + +**A) Single identity:** +`agent= teams= type=__AGENT_TYPE__ project=` +→ Remember AGENT and TEAMS, then go to **Execute**. + +**B) Multiple identities:** +`multiple=true agents= teams= type=__AGENT_TYPE__ project=` +→ Ask the user which agent name to use for this session, then go to **Execute**. + +**C) Not in a team:** +`not_joined=true available_teams=` (or `available_teams=none`) +→ Show the user the available teams from the output, then: + + Before first-time setup, inspect the user's request. If they ask to join, import, or bring in a team that already exists on a server, do not call `join.sh`. Go directly to `remote pull` under Execute. First run `~/.agents/skills/__SKILL_NAME__/scripts/team-list.sh --json --scope all`; if a same-named local team has `binding_state` `none` or `disconnected`, stop and ask the user how to proceed. After pull succeeds, return to Identity setup so the user can register a new local agent in the pulled team. + + > **First-time setup required.** + > Joining a team so this agent can send and receive messages. + > - **Team name**: a group of agents that can message each other (available: ) + > - **Agent name**: this agent's identity within the team + + 1. Ask: "Enter a team name (joins existing or creates new)" + 2. If the team name given already appears in `available_teams`, run `~/.agents/skills/__SKILL_NAME__/scripts/team.sh ` to see the current roster (name, type, project) and note the names already in use. Look for a naming convention already in play (e.g. a shared base name with role and number suffixes (`-`), or names derived from the team name) and, when one exists, propose 2-3 unused names that extend it; otherwise propose 2-3 short, distinctive identity names (not a bare tool-type label like `codex`/`cc`). Either way, names must not collide with the roster. Then ask: "Enter a name for this agent (suggestions: , , — or type your own)". For a brand-new team, skip the roster check and just ask: "Enter a name for this agent". + 3. **You MUST use join.sh** — run: `~/.agents/skills/__SKILL_NAME__/scripts/join.sh __AGENT_TYPE__ "$(pwd)"` + 4. Show the result and explain: + + > **Joined!** You can now use `__CMD_PREFIX____SKILL_NAME__` to check and send messages. + > - `__CMD_PREFIX____SKILL_NAME__` — check inbox + > - `__CMD_PREFIX____SKILL_NAME__ send ` — send a message + > - `__CMD_PREFIX____SKILL_NAME__ team` — list team members + > - `__CMD_PREFIX____SKILL_NAME__ history` — message history + + + + + 6. Then check inbox for the newly joined team. + +**D) Suggestions for reuse:** +`suggest=true agents= teams= type=__AGENT_TYPE__ project= available_teams=` +→ No exact registration exists for this project, but there are same-type agent names registered elsewhere. + + 1. Show the suggested agent names to the user. + 2. Ask whether to reuse one of those names or choose a new one. + 3. Ask for the team name to join (existing or new). + 4. Run: `~/.agents/skills/__SKILL_NAME__/scripts/join.sh __AGENT_TYPE__ "$(pwd)"` + 5. Then continue with the normal post-join flow above. + +## Execute + +**Only use scripts in `~/.agents/skills/__SKILL_NAME__/scripts/` — do not read or modify files under `teams/` or `db/` directly.** Treat the storage layout as internal: never construct a database path or invoke `sqlite3` directly. The scripts resolve the active store, including `AGMSG_STORAGE_PATH` overrides. + +**Terminal/pane self-awareness.** Asked about this session's own terminal, pane, or driver — or before using `arrange`, `peek`, or `poke` below — run `where.sh` (see the "where" argument below) first and answer from its `terminal=`/`capabilities=` fields. Never infer the driver from environment variables or a `grep`/`ps` guess: that is how a session under a real driver ends up reporting a false negative about its own placement, or claiming a capability or a whole driver does not exist when it does (#1171). Each driver's own operational detail lives in `~/.agents/skills/__SKILL_NAME__/scripts/drivers/terminals//README.md`, named by `where.sh`'s own `terminal=` field — never guessed at from a remembered syntax. + +Asked about a *teammate's* placement or status, or what can be done to one, that is `team.sh `'s question (see the "team" argument below), not something to infer from a stale memory of their last known pane. Act on a teammate with `peek.sh`/`poke.sh`/`arrange.sh ` directly rather than guessing reachability first — its exit code says whether it worked and, if not, why (see the "peek"/"poke"/"arrange" arguments below). + +**If no arguments provided (DEFAULT action — always do this when the command is invoked without arguments):** +1. **IMMEDIATELY** run inbox check for each TEAM: `~/.agents/skills/__SKILL_NAME__/scripts/inbox.sh $TEAM $AGENT` +2. Do NOT ask the user what to do — just run the inbox check. +3. If there are messages, read and respond appropriately. To reply: + `~/.agents/skills/__SKILL_NAME__/scripts/send.sh $TEAM $AGENT ""` + + + + +If argument is "history": +1. Run: `~/.agents/skills/__SKILL_NAME__/scripts/history.sh $TEAM $AGENT` + +If argument starts with "team list" (e.g. "team list", "team list --json", "team list --scope project"): +1. Run: `~/.agents/skills/__SKILL_NAME__/scripts/team-list.sh ` +2. This is a distinct command from bare "team" below — check for "team list" FIRST so "list" is never mistaken for a team name. + +If argument is "team" or "team --json": +1. For each TEAM, run: `~/.agents/skills/__SKILL_NAME__/scripts/team.sh $TEAM [--json]`, preserving the option when present. `--json` returns every observed field. +2. This is read-only. It reports each member's identity cells and whether they are consistent — including a member that does not answer at all — but writes nothing and pokes no one. A seat that is wrong or unresponsive is not something this command, or any other, repairs from the outside: typing into another seat's session to fix it is exactly the mistake that used to happen here, and it is gone on purpose, not replaced by another form of the same thing. A member repairs its own identity cells by running `fix` (below), from itself. A seat that cannot or will not do that gets despawned and restarted, or a person takes it — not patched over from another pane. + +If argument starts with "send" (e.g. "send misaki check the server"): +1. Parse target agent and message from the arguments +2. Determine which team the target agent belongs to, then run: + `~/.agents/skills/__SKILL_NAME__/scripts/send.sh $TEAM $AGENT ""` + +If argument is "config": +1. Run: `~/.agents/skills/__SKILL_NAME__/scripts/config.sh show` +2. Show the output to the user. + +If argument starts with "config set" (e.g. "config set hook.check_interval 30"): +1. Parse key and value from the arguments. +2. Run: `~/.agents/skills/__SKILL_NAME__/scripts/config.sh set ` + +If argument is "version": +1. Run: `~/.agents/skills/__SKILL_NAME__/scripts/version.sh` +2. Show the output — the installed version (git-describe provenance recorded at install time). + +If argument is "where" (e.g. asked to report this session's own pane or placement): +1. Run: `~/.agents/skills/__SKILL_NAME__/scripts/where.sh` +2. Report exactly what it prints. Do not try to answer this by naming a terminal yourself or running any terminal-specific command directly — this call already asked every driver on this session's behalf. +3. `resolved=true placement=:` is a known pane; `resolved=true placement=none` is a GENUINE negative (this session's own terminal confirmed it has no addressable pane). `resolved=false` means placement could NOT be determined — `reason` names which terminal(s) were asked and why. Never report a `resolved=false` answer as "no pane" or "not attached to a pane"; those are different answers to different questions, and the difference is the entire point of this command (#1171). +4. `where.sh`'s output also carries `capabilities=` (#1082) — that resolved terminal's own manifest, space-separated, verbatim. Before using `arrange`, `peek`, or `poke` below, check that the verb is in this list; if it is not, report it as unavailable for this terminal (name the terminal) rather than attempting it and finding out from an exit code. If it IS listed, read that terminal's own file — `~/.agents/skills/__SKILL_NAME__/scripts/drivers/terminals//README.md` — before reporting a peek/poke/arrange failure: exit-code meanings differ by driver, and that file, not this one, is where they live. + + + +If argument starts with "actas" followed by an agent name: +1. Parse the new role name and inspect the team roster when suggestions are needed. +2. Run `~/.agents/skills/__SKILL_NAME__/scripts/identities.sh "$(pwd)" __AGENT_TYPE__`. +3. If needed, join with `~/.agents/skills/__SKILL_NAME__/scripts/join.sh __AGENT_TYPE__ "$(pwd)"`. +4. Set the session's active FROM to `` for subsequent sends. +5. Tell the user which role is active. + + +If argument starts with "drop" followed by an agent name: +1. Run `~/.agents/skills/__SKILL_NAME__/scripts/reset.sh "$(pwd)" __AGENT_TYPE__ `. +2. Clear the active role when it matches `` and report the result. + + +If argument starts with "spawn" (e.g. "spawn claude-code alice", "spawn codex reviewer --window"): +1. Parse `` (a spawnable agent type), ``, and any options (`--boot-prompt `, `--project `, `--team `, `--window`, `--split h|v`, `--terminal