diff --git a/.claude-plugin/plugin.json b/.claude-plugin/plugin.json index 60a7a851d..fa256a2c9 100644 --- a/.claude-plugin/plugin.json +++ b/.claude-plugin/plugin.json @@ -1,7 +1,7 @@ { "name": "agmsg", "description": "Cross-agent messaging via SQLite. Send messages between CLI AI agents. No daemon, no network.", - "version": "1.2.0", + "version": "1.2.1", "author": { "name": "fujibee" }, diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index 04e590d2b..0e8c4fe44 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -58,6 +58,21 @@ 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 + # The Windows legs' sqlite3, pinned three ways (#824). + # + # This comment used to end "one trip to chocolatey, which is the only time + # the community feed is asked at all". There is no feed any more: the binary + # comes from one sqlite.org URL, and the hash is what makes the pin mean + # something rather than the version string alone. + # + # Bumping a release means changing all four together — YEAR and BUILD form + # the URL, VERSION is what `sqlite3 --version` will report, and SHA256 is the + # measured digest of that exact zip. A stale SHA256 fails the download step + # loudly, which is the intended way to find out one of them was missed. + WINDOWS_SQLITE_VERSION: '3.53.4' + WINDOWS_SQLITE_YEAR: '2026' + WINDOWS_SQLITE_BUILD: '3530400' + WINDOWS_SQLITE_SHA256: 'f46ee2475de4cbe287e6e5f7d43c838796b14e7379cd216bdbb28d391429f9fc' jobs: # Cheap gate: is this PR's diff entirely documentation? Pushes to main always @@ -575,14 +590,85 @@ jobs: if: needs.changes.outputs.docs_only == 'true' run: echo "Diff is documentation-only; skipping the Windows bats legs." - - name: Install sqlite3 + # NO PACKAGE FEED ON THESE LEGS AT ALL (#824). + # + # This used to be `choco install sqlite`. On 2026-08-15 the chocolatey + # community feed answered 503/504 for hours and every Windows leg went + # red for a reason that had nothing to do with the change under test — + # and worse, WHICH step went red depended on which phase of chocolatey's + # two-phase resolution happened to fail, so the same outage was not + # reproducible in place. Measured: `installed 0/1` exits non-zero and + # reddens the install; `installed 0/0` exits ZERO and the red lands three + # steps later on `Run tests`, reading as the diff's fault. + # + # #827 made that legible — retry, cache, and a presence check that is the + # only step allowed to be red about it. This removes the cause instead. + # The issue's own Directions ranked the retry last, "the cheapest change + # and the least durable", and it was right: on a cache miss the leg still + # needed the feed. `bats` is a required check on `main` with + # enforce_admins, so a red Windows leg now blocks every landing. + # + # What is gone is the FEED, not the third party: sqlite.org is still + # somebody else's host. What it is not is a package index — no resolver, + # no two-phase lookup, no per-request 503 that serves one concurrent job + # and refuses another. One pinned URL, and a hash that says the bytes are + # the ones this pin was measured against. + # + # Not measured: whether the GitHub Windows image already ships sqlite3. + # It does not appear in the Windows 2022 or 2025 image manifests, which + # is the published list rather than a `where sqlite3` on a live runner. + - name: Restore sqlite3 (cache) if: needs.changes.outputs.docs_only != 'true' && matrix.sqlite + id: sqlite-cache + uses: actions/cache@v4 + with: + path: C:\sqlite-tools + key: sqlite3-${{ runner.os }}-sqliteorg-${{ env.WINDOWS_SQLITE_VERSION }} + + - name: Fetch sqlite3 from sqlite.org + if: needs.changes.outputs.docs_only != 'true' && matrix.sqlite && steps.sqlite-cache.outputs.cache-hit != 'true' shell: pwsh - run: choco install sqlite -y --no-progress + run: | + $ErrorActionPreference = 'Stop' + $url = "https://sqlite.org/$env:WINDOWS_SQLITE_YEAR/sqlite-tools-win-x64-$env:WINDOWS_SQLITE_BUILD.zip" + $zip = Join-Path $env:RUNNER_TEMP 'sqlite-tools.zip' + # Three attempts: one host having a bad second is still possible, it + # is just no longer a package index having a bad phase. + for ($attempt = 1; $attempt -le 3; $attempt++) { + try { + Invoke-WebRequest -Uri $url -OutFile $zip -UseBasicParsing + break + } catch { + Write-Host "::warning::sqlite.org fetch failed on attempt $attempt" + Start-Sleep -Seconds (10 * $attempt) + } + } + if (-not (Test-Path $zip)) { exit 0 } # the presence check below decides the colour + # THE HASH IS THE POINT. Without it this trades a feed that answers + # errors for a host that could answer anything, and the tests would + # run against whatever arrived. + $actual = (Get-FileHash -Path $zip -Algorithm SHA256).Hash.ToLower() + if ($actual -ne $env:WINDOWS_SQLITE_SHA256) { + Write-Host "::error::sqlite-tools zip hash mismatch. expected $env:WINDOWS_SQLITE_SHA256, got $actual" + exit 1 + } + Expand-Archive -Path $zip -DestinationPath 'C:\sqlite-tools' -Force - - name: Put chocolatey shims on PATH (Git Bash form) + - name: Put sqlite3 on PATH (Git Bash form) if: needs.changes.outputs.docs_only != 'true' && matrix.sqlite - run: echo "/c/ProgramData/chocolatey/bin" >> "$GITHUB_PATH" + run: echo "/c/sqlite-tools" >> "$GITHUB_PATH" + + # THE STEP THAT IS ALLOWED TO BE RED ABOUT THIS, kept from #827 and still + # the load-bearing part: whatever supplies sqlite3, the step that decides + # the leg's colour must be the one that says the dependency is missing, + # not the one that runs the tests. + - name: sqlite3 must be present, or this leg is about its download + if: needs.changes.outputs.docs_only != 'true' && matrix.sqlite + run: | + if ! sqlite3 --version; then + echo "::error::sqlite3 is missing — the pinned sqlite.org download did not arrive. This leg reports on that, NOT on the change under test — see #824." + exit 1 + fi - name: Install bats if: needs.changes.outputs.docs_only != 'true' @@ -595,7 +681,23 @@ jobs: if: needs.changes.outputs.docs_only != 'true' run: | bash --version | head -1 - sqlite3 --version || sqlite3.exe --version || echo "sqlite3 not found on PATH" + # Two changes here, pulling in opposite directions (#824). + # + # The `|| sqlite3.exe --version || echo "sqlite3 not found on PATH"` + # fallback is gone: it turned a missing binary into a success line, so + # nothing between a failed install and `Run tests` said anything was + # wrong. Absence is decided by the presence check above, which is + # allowed to be red about it. + # + # Without the fallback this line would fail on a leg that never + # installs sqlite3 — `driver input (#817)` carries `sqlite: false` + # precisely so a package feed cannot stop a leg driving a mock bash + # driver. Reporting a version it was never meant to have would + # re-create, one step lower, the coupling #822 removed. + # `matrix.sqlite` is a fixed value from the matrix above. + if [ "${{ matrix.sqlite }}" = "true" ]; then + sqlite3 --version + fi bats --version - name: Run tests diff --git a/CHANGELOG.md b/CHANGELOG.md index 9ffbde7b8..cb5d3c8f5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,17 @@ All notable changes to this project are documented here. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). +## [1.2.1] - 2026-08-18 + +### Added +- Windows can use the hosted service: connect reaches the server, the fingerprint is real, and doctor names a wedged lock (#868) + +### Fixed +- Bound the wait on the local child without spawning anything to do it (#821) +- Record provenance when git answers in another path space (#830) (#842) +- Stop telling every agent that key rotate is unavailable (#841) +- The guard was on the wrong job's step, and it silenced a real one + ## [1.2.0] - 2026-08-15 ### Fixed diff --git a/CONTRIBUTING.ja.md b/CONTRIBUTING.ja.md index a353239fb..b118aa1e9 100644 --- a/CONTRIBUTING.ja.md +++ b/CONTRIBUTING.ja.md @@ -22,6 +22,7 @@ 3. テストスイートを実行する: `bats tests/`。 4. 周囲のコードスタイルに合わせる。Bash が主要言語であり、すべてのスクリプトの先頭で `set -euo pipefail` を使うこと。 5. ユーザーから見える変更であれば、ドキュメントも更新する。 +6. PR は `main` に squash コミット 1 つとして着地する — リポジトリ側で merge と rebase のマージは無効にしてある。ブランチのコミットは 1 つにまとめられるので、PR のタイトルはそのままコミットの件名として読める形で書くこと。レビュー前にブランチの履歴を整える必要はない。 ## リリース diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 1d5df919a..a86a607b3 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -22,6 +22,7 @@ Open an issue on [`fujibee/agmsg`](https://github.com/fujibee/agmsg/issues). Inc 3. Run the test suite: `bats tests/`. 4. Match the surrounding code style. Bash is the primary language; use `set -euo pipefail` at the top of every script. 5. Update docs if the change is user-visible. +6. PRs land on `main` as a single squash commit — merge and rebase merges are disabled on the repository. Your branch's commits are collapsed into one, so write the PR title to read well as that commit's subject. There is no need to tidy your branch history before review. ## Releases diff --git a/SKILL.md b/SKILL.md index 9bd5a4fe8..cf388e4b9 100644 --- a/SKILL.md +++ b/SKILL.md @@ -307,8 +307,24 @@ If argument starts with "key import" followed by a team name: 3. Do not offer an environment-variable path. An identity file is a permanent secret; always use the human-in-own-terminal flow above. -`key rotate` and device-pairing `key request`/`key approve` are not available -yet. If the user asks for one, tell them so instead of attempting to run it. +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/agmsg/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. ## Permission prompts (Claude Code) diff --git a/VERSION b/VERSION index 26aaba0e8..6085e9465 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -1.2.0 +1.2.1 diff --git a/cliff.toml b/cliff.toml index cf3c93ebc..ced1013b8 100644 --- a/cliff.toml +++ b/cliff.toml @@ -74,6 +74,7 @@ commit_parsers = [ { message = "^Fix Codex monitor multi-identity delivery", group = "Fixed" }, { message = "^Isolate Codex monitor bridges by role", group = "Added" }, { message = "^Allow launcher-reserved bridge PID", group = "Fixed" }, + { message = "^Stop telling every agent that key rotate", group = "Fixed" }, # Everything else (merges, initial commit, misc) is intentionally dropped. { message = ".*", skip = true }, ] diff --git a/install.sh b/install.sh index 4aa3e6f88..4f542d413 100755 --- a/install.sh +++ b/install.sh @@ -36,7 +36,7 @@ AGENTS_DIR="$HOME/.agents" # uncommitted changes. Non-git (tarball via setup.sh/npx, no .git): fall back to # the canonical VERSION file. See #117. agmsg_source_version() { - local v top + local v top native # Only describe when SCRIPT_DIR is ITS OWN git checkout. `git describe` # searches ancestors for a .git, so a non-git copy unpacked under some other # git repo would otherwise record that PARENT repo's describe instead of @@ -51,7 +51,34 @@ agmsg_source_version() { # app's own version comparison (agmsg_core_version_status in agmsg.rs) # can't parse as semver, which it then treats as "outdated" unconditionally. top="$(git -C "$SCRIPT_DIR" rev-parse --show-toplevel 2>/dev/null || true)" - if [ -n "$top" ] && [ "$top" = "$SCRIPT_DIR" ] \ + # THE TWO SIDES ARE IN DIFFERENT PATH SPACES ON WINDOWS, so the equality was + # always false there and every Git Bash install recorded the VERSION file + # instead of the describe string (#830): + # + # $SCRIPT_DIR /tmp/tmp.XXXX/agmsg MSYS form, from bash + # git --show-toplevel C:/Users/.../tmp.XXXX/agmsg native form, from git + # + # `cygpath -m` is the mixed form git reports — the same second chance this + # file already takes for the writable paths below, and the same one + # `agmsg_cmdline_names_path` takes in compat.sh, where the identical mismatch + # made four watcher-ownership checks answer "not ours" on Windows. + # + # The condition below is a CAPABILITY, not an operating system: where cygpath + # is not on PATH, `native` stays empty and this is the plain comparison and + # nothing else. Saying "off Windows" instead would be wider than the code — + # this file's own test drives the second branch on macOS and Linux by putting + # a cygpath stub on PATH. + # + # Where cygpath is absent, fails, returns nothing, or returns a path unequal + # to git's toplevel, the recorded value is the fallback, exactly as before. + # A wrong answer that happened to equal the toplevel would still take the + # describe branch, so this is a set of conditions and not a guarantee that + # the worst case is the old behaviour. + native="" + if command -v cygpath >/dev/null 2>&1; then + native="$(cygpath -m "$SCRIPT_DIR" 2>/dev/null || true)" + fi + if [ -n "$top" ] && { [ "$top" = "$SCRIPT_DIR" ] || { [ -n "$native" ] && [ "$top" = "$native" ]; }; } \ && v="$(git -C "$SCRIPT_DIR" describe --tags --always --dirty --abbrev=7 --match 'v[0-9]*' 2>/dev/null)" \ && [ -n "$v" ]; then printf '%s' "$v" @@ -68,6 +95,18 @@ UPDATE_ONLY=false INTERACTIVE=true AGENT_TYPE="" # claude-code, codex, gemini, antigravity — passed via --agent-type, or empty for auto/default +# Types the installer renders their OWN shared SKILL.md for (their template.md +# differs from codex's). Everything else -- codex itself, plus claude-code and +# copilot, which keep separate dedicated copies elsewhere -- gets the codex- +# typed shared SKILL.md. One list, read by three call sites below (fresh +# install's template pick, --update's template pick, and --update's type +# re-detection from the SKILL.md already on disk): before #846, the third site +# hardcoded its own, narrower copy of this same set (missing opencode/hermes/ +# 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" + configure_codex_sandbox() { # --- Configure Codex sandbox (if Codex is installed) --- # The Codex bridge writes pidfiles/sockets/request files under the @@ -380,22 +419,28 @@ if [ "$UPDATE_ONLY" = true ]; then CMD_NAME="$SKILL_NAME" echo " Updating $SKILL_NAME..." if [ -z "$AGENT_TYPE" ]; then - if grep -q "whoami.sh.*antigravity" "$SKILL_DIR/SKILL.md" 2>/dev/null; then - AGENT_TYPE="antigravity" - elif grep -q "whoami.sh.*gemini" "$SKILL_DIR/SKILL.md" 2>/dev/null; then - AGENT_TYPE="gemini" - elif grep -q "whoami.sh.*grok-build" "$SKILL_DIR/SKILL.md" 2>/dev/null; then - AGENT_TYPE="grok-build" - else - AGENT_TYPE="codex" - fi + # Re-detect the type this install's shared SKILL.md was last rendered for, + # from the whoami.sh line its own template prints (#846) -- every + # renderable type's line is unambiguous against every other's; see the + # cross-grep this list is built from, noted alongside + # AGMSG_SHARED_SKILL_TPL_TYPES above. codex is not grepped for: it is the + # default a match against this list falls back to. + AGENT_TYPE="codex" + for _agmsg_t in $AGMSG_SHARED_SKILL_TPL_TYPES; do + if grep -q "whoami.sh.*$_agmsg_t" "$SKILL_DIR/SKILL.md" 2>/dev/null; then + AGENT_TYPE="$_agmsg_t" + break + fi + done + unset _agmsg_t fi - # The shared SKILL.md uses the codex template by default; gemini/antigravity/ - # opencode get their own. (claude-code and copilot reuse the codex-typed - # shared SKILL.md; their dedicated copies are dropped separately below.) + # The shared SKILL.md uses the codex template by default; the types in + # AGMSG_SHARED_SKILL_TPL_TYPES get their own. (claude-code and copilot reuse + # the codex-typed shared SKILL.md; their dedicated copies are dropped + # separately below.) TPL_TYPE="codex" - case "$AGENT_TYPE" in - gemini|antigravity|opencode|hermes|cursor|grok-build) TPL_TYPE="$AGENT_TYPE" ;; + case " $AGMSG_SHARED_SKILL_TPL_TYPES " in + *" $AGENT_TYPE "*) TPL_TYPE="$AGENT_TYPE" ;; esac sed "s/__SKILL_NAME__/$SKILL_NAME/g" "$(agmsg_type_template_path "$TPL_TYPE")" > "$SKILL_DIR/SKILL.md" # Recursive sync so nested helper dirs (scripts/lib/, scripts/drivers/types/) @@ -536,10 +581,10 @@ mkdir -p "$SKILL_DIR"/{scripts,types,db,agents} # SKILL.md is generated from the agent-specific command template, resolved from # the type manifest (scripts/drivers/types//template.md). The shared SKILL.md uses the -# codex template by default; gemini/antigravity/opencode get their own. +# codex template by default; the types in AGMSG_SHARED_SKILL_TPL_TYPES get their own. TPL_TYPE="codex" -case "$AGENT_TYPE" in - gemini|antigravity|opencode|hermes|cursor|grok-build) TPL_TYPE="$AGENT_TYPE" ;; +case " $AGMSG_SHARED_SKILL_TPL_TYPES " in + *" $AGENT_TYPE "*) TPL_TYPE="$AGENT_TYPE" ;; esac sed "s/__SKILL_NAME__/$CMD_NAME/g" "$(agmsg_type_template_path "$TPL_TYPE")" > "$SKILL_DIR/SKILL.md" # Recursive sync so nested helper dirs (scripts/lib/, scripts/drivers/types/) ship diff --git a/package.json b/package.json index 271703d1b..229754edf 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "agmsg", - "version": "1.2.0", + "version": "1.2.1", "description": "Cross-agent messaging via SQLite for CLI AI agents (Claude Code, Codex, Gemini CLI, GitHub Copilot CLI, Antigravity, OpenCode). The npm package is a thin bootstrapper that fetches and runs the canonical bash installer at https://github.com/fujibee/agmsg.", "bin": { "agmsg": "bin/agmsg.js" diff --git a/scripts/drivers/types/antigravity/template.md b/scripts/drivers/types/antigravity/template.md index 58fe15fb6..50bdb4695 100644 --- a/scripts/drivers/types/antigravity/template.md +++ b/scripts/drivers/types/antigravity/template.md @@ -232,4 +232,11 @@ If argument starts with "key import" followed by a team name: 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. -`key rotate` and device-pairing `key request`/`key approve` are not available yet (they refuse unconditionally and change no state) — if the user asks for either, tell them so rather than attempting to run them. +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/claude-code/template.md b/scripts/drivers/types/claude-code/template.md index fa21854b7..86fc0f441 100644 --- a/scripts/drivers/types/claude-code/template.md +++ b/scripts/drivers/types/claude-code/template.md @@ -337,4 +337,11 @@ If argument starts with "key import" followed by a team name: 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. -`key rotate` and device-pairing `key request`/`key approve` are not available yet (they refuse unconditionally and change no state) — if the user asks for either, tell them so rather than attempting to run them. +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/codex/template.md b/scripts/drivers/types/codex/template.md index 70d0d5ce0..f5e6344c7 100644 --- a/scripts/drivers/types/codex/template.md +++ b/scripts/drivers/types/codex/template.md @@ -266,4 +266,11 @@ If argument starts with "key import" followed by a team name: 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. -`key rotate` and device-pairing `key request`/`key approve` are not available yet (they refuse unconditionally and change no state) — if the user asks for either, tell them so rather than attempting to run them. +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/copilot/template.md b/scripts/drivers/types/copilot/template.md index df8d1f90e..51d62c467 100644 --- a/scripts/drivers/types/copilot/template.md +++ b/scripts/drivers/types/copilot/template.md @@ -232,4 +232,11 @@ If argument starts with "key import" followed by a team name: 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. -`key rotate` and device-pairing `key request`/`key approve` are not available yet (they refuse unconditionally and change no state) — if the user asks for either, tell them so rather than attempting to run them. +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/cursor/template.md b/scripts/drivers/types/cursor/template.md index 79baa8020..4548065d4 100644 --- a/scripts/drivers/types/cursor/template.md +++ b/scripts/drivers/types/cursor/template.md @@ -235,4 +235,11 @@ If argument starts with "key import" followed by a team name: 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. -`key rotate` and device-pairing `key request`/`key approve` are not available yet (they refuse unconditionally and change no state) — if the user asks for either, tell them so rather than attempting to run them. +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/gemini/template.md b/scripts/drivers/types/gemini/template.md index 22a2d3e32..a817f077f 100644 --- a/scripts/drivers/types/gemini/template.md +++ b/scripts/drivers/types/gemini/template.md @@ -232,4 +232,11 @@ If argument starts with "key import" followed by a team name: 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. -`key rotate` and device-pairing `key request`/`key approve` are not available yet (they refuse unconditionally and change no state) — if the user asks for either, tell them so rather than attempting to run them. +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/grok-build/template.md b/scripts/drivers/types/grok-build/template.md index c6eddebe5..7a088f80e 100644 --- a/scripts/drivers/types/grok-build/template.md +++ b/scripts/drivers/types/grok-build/template.md @@ -263,4 +263,11 @@ If argument starts with "key import" followed by a team name: 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. -`key rotate` and device-pairing `key request`/`key approve` are not available yet (they refuse unconditionally and change no state) — if the user asks for either, tell them so rather than attempting to run them. +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/hermes/template.md b/scripts/drivers/types/hermes/template.md index 6ffa08802..7828ea66b 100644 --- a/scripts/drivers/types/hermes/template.md +++ b/scripts/drivers/types/hermes/template.md @@ -220,4 +220,11 @@ If argument starts with "key import" followed by a team name: 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. -`key rotate` and device-pairing `key request`/`key approve` are not available yet (they refuse unconditionally and change no state) — if the user asks for either, tell them so rather than attempting to run them. +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/opencode/template.md b/scripts/drivers/types/opencode/template.md index 94b1db2b7..6eb7f29f4 100644 --- a/scripts/drivers/types/opencode/template.md +++ b/scripts/drivers/types/opencode/template.md @@ -260,4 +260,11 @@ If argument starts with "key import" followed by a team name: 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. -`key rotate` and device-pairing `key request`/`key approve` are not available yet (they refuse unconditionally and change no state) — if the user asks for either, tell them so rather than attempting to run them. +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/internal/roster-sync-driver.sh b/scripts/internal/roster-sync-driver.sh index b8b409db5..3f08ff2a9 100755 --- a/scripts/internal/roster-sync-driver.sh +++ b/scripts/internal/roster-sync-driver.sh @@ -7,6 +7,15 @@ SKILL_DIR="$(cd "$SCRIPT_DIR/../.." && pwd)" source "$SKILL_DIR/scripts/lib/registry-lock.sh" # shellcheck disable=SC1091 source "$SKILL_DIR/scripts/lib/roster-journal.sh" +# For `_agmsg_pid_alive_local`, which is where liveness is OWNED. A bare +# `kill -0` here would be a second answer to "is it running?" beside the one +# that file exists to give -- and a repo-wide check refuses one, correctly. +# shellcheck disable=SC1091 +source "$SKILL_DIR/scripts/lib/instance-id.sh" +# For `compat_get_cmdline`, which is how `scripts/remote.sh` already answers +# "is this pid still the process I started?" on every platform this ships to. +# shellcheck disable=SC1091 +source "$SKILL_DIR/scripts/lib/compat.sh" operation="${1:?Missing operation}"; team="${2:?Missing team}" server="${3:?Missing server id}"; remote="${4:?Missing remote team id}" @@ -20,6 +29,32 @@ protocol="${5:?Missing protocol version}"; shift 5 config="${AGMSG_SYNC_LOCAL_ROSTER_FILE:-$SKILL_DIR/teams/$team/config.json}" team_dir="$(cd "$(dirname "$config")" && pwd)" +ROSTER_SYNC_BUDGET_S="${AGMSG_ROSTER_SYNC_TIMEOUT_S:-120}" +# A BOUND THAT CANNOT BE READ IS NOT A BOUND. `read -t` rejects a zero, a +# negative or a non-numeric budget by failing immediately, and its failure is +# indistinguishable here from "the writer is gone" — so a mistyped setting +# would turn the ceiling off and leave the wait unbounded, silently, which is +# the defect this file is closing (raised in review). Checked before anything +# is started — and before the lock is taken, because refusing after the lock +# and after `agmsg_roster_ensure` would mean a setting error had already moved +# the team's state and taken the critical section (raised in review). +# The length is checked with the digits, because a value can be all digits and +# still be unusable: `[ "$x" -le 0 ]` on a thirty-digit number is beyond what +# the shell's integers hold, and it errors — under `set -e` that ends this +# script with the shell's own status and none of the sentence below, which is +# the silent refusal this guard exists to remove (raised in review). Nine +# digits is over thirty years in seconds; nothing legitimate reaches it. +case "$ROSTER_SYNC_BUDGET_S" in + ''|*[!0-9]*|??????????*) + echo "agmsg: roster sync $operation failed for team '$team': AGMSG_ROSTER_SYNC_TIMEOUT_S must be a positive whole number of seconds, at most nine digits, got '${AGMSG_ROSTER_SYNC_TIMEOUT_S:-}'" >&2 + exit 15 ;; +esac +if [ "$ROSTER_SYNC_BUDGET_S" -le 0 ]; then + echo "agmsg: roster sync $operation failed for team '$team': AGMSG_ROSTER_SYNC_TIMEOUT_S must be greater than zero, got '$ROSTER_SYNC_BUDGET_S'" >&2 + exit 15 +fi + + agmsg_lock_acquire "$team_dir" # agmsg_lock_acquire already installs EXIT cleanup and exit-on-INT/TERM traps. # Keep those handlers: replacing them with release-only handlers would let a @@ -28,8 +63,561 @@ trap 'agmsg_lock_release; exit 129' HUP agmsg_roster_ensure "$team_dir" "$config" node_bin="${AGMSG_SYNC_NODE_BIN:-${AGMSG_NODE:-node}}" -"$node_bin" "$SCRIPT_DIR/roster-sync.mjs" "$operation" "$config" \ - "$server" "$remote" "$protocol" "$@" + +# "Is the recorded pid still the process this operation started?" — asked in +# ONE place, so the signalling path and the release decision cannot come to +# different answers. Two questions with one wording is how a gate ends up +# guarding something narrower than the thing it authorises. +# +# A NUMBER IN A FILE IS NOT AN IDENTITY (raised as BLOCKING, and the answer +# was already in this repo). A pid is recycled the moment its process is +# reaped, so between the wrapper writing that number and the timeout path +# reading it, it can belong to something else entirely. +# `_remote_sync_engine_status` in `scripts/remote.sh` had already been here: +# "A live PID is not enough: PID reuse can make an unrelated process pass +# kill -0, so running requires the exact engine script/team suffix in argv." +# +# Both halves are required. Liveness alone reads a recycled number as our +# child; argv alone would accept a match on a pid that is already gone. The +# suffix is specific rather than convenient: `$config` is per-team, so a +# roster sync for a DIFFERENT team cannot satisfy it. +# THREE ANSWERS, AND A BOOLEAN CANNOT CARRY THEM (raised in review, twice). +# +# The first version of this returned true/false, so every caller had to read +# "false" as "gone" — and false covered two very different things: the process +# really has exited, and the process is alive but we could not establish that +# it is ours. Releasing the lock is only safe for the first. Collapsing them +# put the unsafe reading on the default path, which is the same shape as every +# other defect in this PR. +# +# So it prints one of three words: +# +# ours alive, and still carrying this operation's argv +# gone not alive — the only answer that authorises a release +# unknown everything else, and it is NOT a residual category: +# * no pid recorded at all (the wrapper never got that far, so +# whether node started is not known — this used to read as +# "gone" and release) +# * alive, argv does not match — a recycled number, OR our child +# with an argv this platform would not hand over +_roster_inner_state() { + if [ -z "${_roster_inner:-}" ]; then + printf 'unknown\n' + return + fi + # "UNUSABLE" AND "DEAD" ARE NOT THE SAME ANSWER (raised in review). + # + # `_agmsg_pid_alive_local` starts by rejecting a pid outside the POSIX + # ceiling and returns 1 for it — the same 1 it returns for "no such + # process". A digits-only value can be unusable: `2147483648` passes the + # crude filter at the call site and is refused here, and reading that + # refusal as `gone` releases the lock on a question that could not be asked. + # So the validator is consulted FIRST, and its refusal is `unknown`. + if ! _agmsg_pid_valid "$_roster_inner" 2147483647; then + printf 'unknown\n' + return + fi + if ! _agmsg_pid_alive_local "$_roster_inner"; then + printf 'gone\n' + return + fi + local cmd + cmd="$(compat_get_cmdline "$_roster_inner" 2>/dev/null || true)" + # THE WHOLE QUESTION LIVES IN `agmsg_roster_argv_is_ours`, and it lives in + # `scripts/lib/roster-journal.sh` rather than here so that its cases can + # drive IT rather than a copy of it. Inline, the only way to reach it was to + # run the driver, so the tests re-implemented the comparison and then + # asserted on this file with `grep` -- which measures a duplicate and a + # spelling, never the thing production calls (raised in review). + # + # What it holds, and why, is documented beside it: the quoted forms are + # enumerated rather than the quotes removed, because a quote is an argument + # boundary and deleting it lets a quoted DATA argument that merely contains + # the triple pass as an invocation; both path alphabets, because + # `/c/Users/...` and `C:/Users/...` are the same path; the ordered triple + # with argument boundaries, because three separate `contains` checks are + # three questions and an ordered substring is still a substring. + if agmsg_roster_argv_is_ours "$cmd" "$SCRIPT_DIR/roster-sync.mjs" "$operation" "$config"; then + printf 'ours\n' + return + fi + printf 'unknown\n' +} + +# Signalling has exactly one safe answer, so it gets its own name rather than +# every call site re-deriving it from the word. +_roster_inner_still_ours() { + [ "$(_roster_inner_state)" = "ours" ] +} + +# THE CRITICAL SECTION IS BOUNDED IN TIME, NOT ONLY IN SCOPE (#821). +# +# The scope is right and stays: `roster-sync.mjs` is the read-modify-write of +# the journal and state this lock exists to serialise. It does no network — +# measured, with a positive control on the search — and its whole input is +# handed over in one write before it starts, so nothing in here waits on a +# remote. Moving it out of the lock would move the very thing being protected. +# +# What was missing is a bound. Release was the EXIT trap alone, which is sound +# when this shell reaches its own exit: a child that fails to start, or exits +# non-zero, still gets there under `set -e`. It is not sound when the child +# neither runs nor returns — the shell waits, the trap never runs, and the lock +# is held by a live process that will never finish. The next start then fails +# on `.config.lock: File exists`, and the team is unusable until someone finds +# a directory they have no reason to know about. +# +# So the wait has a ceiling, and passing it is a failure with a name rather +# than a hang. The traps stay as the backup they always were. +# THE BOUND ADDS NO PROCESS THAT OUTLIVES THE CALL (#821). +# +# The first version put a watchdog beside the child: one more process per +# roster operation. The leading explanation for the field failure is process +# pressure — a spawn refused with a Windows DLL-initialisation status — so a +# guard against hanging that raises the number of spawns can make the thing it +# guards against more likely. Raised in review, and it is the right objection. +# +# So the wait is bounded by a READ, not by a second process. The child is given +# one end of a FIFO it never writes to; when it exits, that end closes and the +# read here returns end-of-file. +# +# This USED TO SAY that `read -t` then distinguishes the two outcomes by +# status — above 128 for the budget, anything else for a departed writer — AND +# THAT WAS WRONG WHERE IT RUNS. The correction and its measurement are below; +# the sentence is kept in the past tense so this file does not appear to state +# two contracts at once. +# +# What this does NOT claim is "no extra process at all": `mktemp`, `mkfifo` and +# `rm` are external commands and each is a spawn (raised in review; an earlier +# version of this comment said "one process runs, exactly as before", which is +# false). They are short-lived and sequential, where a watchdog is concurrent +# and lives as long as the operation — and on a machine that is refusing +# spawns, a process held open beside every roster call is the shape that +# matters. What happens when one of them fails is NOT "fall back to the +# unbounded path" — that is what this used to do, and it is the defect. See +# the mode table below: no FIFO still bounds by polling, and no temp file at +# all refuses. +# A FIFO to notice the child's exit by, and a SENTINEL to say what happened. +# +# Two files, because one of them cannot answer both questions on the shell +# this actually runs under. `read -t` returns 1 for a timeout AND 1 for +# end-of-file on Bash 3.2 — measured on 3.2.57, which is what the macOS +# runners use — so a bound written as `status > 128` is simply never taken +# there. That is what happened: a two-second budget waited twenty-five +# minutes, on the one platform whose bash cannot tell the two apart, while +# every Linux leg stayed green because Bash 5 does distinguish them. +# +# So the ANSWER IS NOT THE RETURN CODE. The wrapper writes its child's exit +# status to a sentinel as its last act; this side reads the sentinel. Present +# means the wrapper finished — including when the command could not be exec'd +# at all, because a failed exec sets `$?` in the wrapper and the wrapper still +# reaches the write. Absent after the wait means the wrapper is still there, +# which is the only case a bound is for. +_roster_fifo="$(mktemp -u 2>/dev/null)" || _roster_fifo="" +_roster_sentinel="$(mktemp 2>/dev/null)" || _roster_sentinel="" +_roster_pidfile="$(mktemp 2>/dev/null)" || _roster_pidfile="" + +# FAILING TO BUILD THE BOUND MUST NOT RUN WITHOUT ONE (raised by three +# reviewers, and they are right). +# +# This used to fall back to the foreground, unbounded call — the exact state +# #821 forbids — and printed `running without a time bound` while doing it. +# That is not a fallback. "The instrument could not be built, so the property +# is dropped" reproduces the defect on precisely the machines least able to +# afford it: a temp directory that is full or unwritable is the same condition +# that makes a child hang, so the two arrive together. +# +# There are two ways to wait with a ceiling, and only the faster one needs a +# FIFO: +# +# fifo a blocking read on a descriptor the wrapper holds. No polling, so no +# spawn while waiting — which is why it is preferred on a machine that +# is refusing spawns. +# poll the sentinel is looked for on a timer. Costs one `sleep` per second +# of waiting, and needs nothing but the two temp files. +# +# Both bound the wait. Only when even `mktemp` fails is there no bound to be +# had, and then this REFUSES: releases the lock and says why, rather than +# taking the critical section for an unlimited time. +_roster_wait=none +if [ -n "$_roster_sentinel" ] && [ -n "$_roster_pidfile" ]; then + if [ -n "$_roster_fifo" ] && mkfifo "$_roster_fifo" 2>/dev/null; then + _roster_wait=fifo + else + _roster_fifo="" + _roster_wait=poll + fi +fi + +# A PARTIAL SETUP LEAVES PART OF ITSELF BEHIND, AND `none` IS WHERE IT SHOWS +# (raised in review). +# +# The three `mktemp` calls above run independently, so "could not build a +# bound" is not one state: the sentinel can exist while the pidfile failed, or +# the other way round. Both routes out of `none` — the override and the +# refusal — used to walk past whatever had already been created, and the +# failure that sends a run down here is *a temp directory that is full*. A +# feature that responds to a full filesystem by adding a file to it is the +# wrong shape, and it is the same self-defeating loop `sync-autostart.sh` +# already carries a comment about. +# +# Swept once, here, before either route is taken. `rm -f` on an empty name is +# a no-op, so the three cases (none created, one, two) need no branching, and +# `|| true` because nothing below may hang on a failed unlink. +if [ "$_roster_wait" = "none" ]; then + rm -f "$_roster_sentinel" "$_roster_pidfile" 2>/dev/null || true +fi + +# THE WAY OUT, because a refusal with no override is a wall. An operator who +# knows their filesystem cannot hold a temp file, and would rather risk the +# lock than not sync at all, can say so — deliberately, by name, in the +# environment. What is removed is the SILENT return to the old behaviour. +if [ "$_roster_wait" = "none" ] && [ "${AGMSG_ROSTER_SYNC_UNBOUNDED:-0}" = "1" ]; then + echo "agmsg: roster sync $operation for team '$team': AGMSG_ROSTER_SYNC_UNBOUNDED=1 — running the local roster child with NO time bound; if it hangs it holds the team lock until this process is killed" >&2 + "$node_bin" "$SCRIPT_DIR/roster-sync.mjs" "$operation" "$config" \ + "$server" "$remote" "$protocol" "$@" + case "$operation" in + reconcile|apply) agmsg_roster_project_config "$team_dir" "$config" ;; + esac + exit 0 +fi + +if [ "$_roster_wait" = "none" ]; then + agmsg_lock_release + echo "agmsg: roster sync $operation failed for team '$team': cannot create the temporary files needed to bound the local roster child (is TMPDIR set to a path that exists and is writable, and is the filesystem full?); refusing rather than holding the team lock for an unlimited time. To run it anyway, set AGMSG_ROSTER_SYNC_UNBOUNDED=1." >&2 + exit 16 +fi + +# One spawn, whichever wait follows: when there is no FIFO the wrapper's fd 8 +# goes to /dev/null, so the line that starts the child is the same in both +# modes and its descriptor closing cannot drift between them. +_roster_write_target="/dev/null" +if [ "$_roster_wait" = "fifo" ]; then + _roster_write_target="$_roster_fifo" +fi + +# Both remaining modes are bounded; `none` has already left, one way or the +# other. The block is kept so the two waits sit inside one spawn/teardown. +if [ "$_roster_wait" != "none" ]; then + # fd 9 is this shell's stdin, kept for the child: a shell with job control + # off gives an asynchronous command /dev/null for stdin, and backgrounding + # the child silently took away the records the engine had already written to + # fd 0. Measured, as every operation failing with "input is invalid". + exec 9<&0 + # The wrapper records the READ CHILD'S pid before waiting on it, because the + # thing that has to be stopped on a timeout is the command, not the shell + # around it. Signalling the wrapper alone leaves the command orphaned, still + # holding this shell's stdout — and a caller that captures output then waits + # for an end-of-file that never comes. That is the same "an abandoned child + # keeps the caller's streams" defect fixed twice tonight, and it reappeared + # here: measured, as a three-second budget that had not returned after + # seventeen minutes. + { + # `3>&- 4>&-` HERE AS WELL AS ON THE GROUP, and the repetition is the point. + # The enclosing group already closes both, so node's descriptors were never + # actually at risk — but `tests/test_spawn_fd_guard.bats` reads the spawn + # LINE, deliberately: an exemption keyed on "something upstream closes them" + # accepts an `exec` inside a branch that never runs, or after the spawn it + # was supposed to cover. Redundancy is the cheaper mistake, and the same + # belt-and-braces pattern is already in `scripts/lib/sync-autostart.sh`. + "$node_bin" "$SCRIPT_DIR/roster-sync.mjs" "$operation" "$config" \ + "$server" "$remote" "$protocol" "$@" <&9 9<&- 3>&- 4>&- & + _rs_node=$! + # THE WRAPPER'S OWN COPY, AND IT IS A THIRD ONE. Introducing this shell to + # carry the pid put a process between the driver and node that inherits + # fd 9 and was closing it nowhere — so the caller's stdin was held for the + # whole operation by something neither the child's `9<&-` nor the parent's + # `exec 9<&-` reaches. The same class as the two closes around it, made + # reachable again by the shell added to fix a different one. + # + # CI found this, and the case that found it is the behavioural half whose + # comment said `$PPID` is the driver. It stopped being the driver when this + # brace group appeared; the assertion then read THIS shell's table and + # reported fd 9 open, which was the truth. + # + # Closed here rather than in the group's redirection list, because node + # above still needs it: `<&9` is read when that command starts. + exec 9<&- + printf '%s\n' "$_rs_node" > "$_roster_pidfile" + _rs_rc=0 + wait "$_rs_node" || _rs_rc=$? + printf '%s\n' "$_rs_rc" > "$_roster_sentinel" + } 3>&- 4>&- 8> "$_roster_write_target" & + _roster_child=$! + # BOTH COPIES GO. `<&9` duplicates the caller's stdin onto fd 0 and leaves + # fd 9 open beside it, so node would hold that stream twice and this shell + # would hold it until its own exit — the same "a child keeps the caller's + # streams" class fixed twice tonight, reintroduced by the descriptor used to + # fix it (raised in review). The child closes its saved copy in the + # redirection above; this closes ours the moment it is no longer needed. + exec 9<&- + if [ "$_roster_wait" = "fifo" ]; then + # The two opens rendezvous: a writer's `8>` does not complete until a reader + # arrives, so this cannot be left waiting for a writer that has already gone. + exec 8< "$_roster_fifo" + # `|| true` because this file runs under `set -e` and `rm` is an external + # command: a spawn refused, or a filesystem that will not unlink, would end + # this shell HERE — while the child is still running — and the EXIT trap + # would release the lock beside a live writer. That is worse than the leak + # being fixed, and it is the one place this fix could create it (raised in + # review). The FIFO is already open on fd 8; unlinking it is tidiness, and + # tidiness may not decide whether the lock is held. + rm -f "$_roster_fifo" || true + + # The read's own status is DELIBERATELY DISCARDED. It cannot be trusted to + # separate "the budget expired" from "the writer is gone" on Bash 3.2, and + # trusting it there is the defect this replaces. + read -r -t "$ROSTER_SYNC_BUDGET_S" _roster_ignored <&8 || true + exec 8<&- + else + # NO FIFO, AND STILL BOUNDED. The wrapper's fd 8 went to /dev/null, so + # there is nothing to read; what is watched instead is the sentinel the + # wrapper writes as its last act — the same fact, asked on a timer rather + # than by blocking. + # + # `-s` and not `-e`: the file was created empty by `mktemp`, so its mere + # existence says nothing. It is non-empty exactly once the status is in it. + # + # The cost is one `sleep` per second of waiting, and that is why this is + # the second choice rather than the only one — on a machine refusing + # spawns, a wait that spawns is the wrong shape. It is still the right + # trade against no bound at all. + # + # `SECONDS` is the shell's own counter, so the deadline needs no `date`. + # + # `|| true` ON THE SLEEP, for the same reason the `rm` above has one, and + # it is sharper here (raised in review). `sleep` is an external command + # under `set -e`; a refused spawn would end this shell HERE — with the + # wrapper and node already started and none of the terminate/KILL/reap + # below reached — and the EXIT trap would drop the lock beside a live + # writer. That is the contract this path is written to keep, broken by the + # one command it uses to wait. And this mode is *chosen* on machines that + # could not make a FIFO, which is the same population that refuses spawns. + # + # What a failing `sleep` costs instead: this loop spins on `SECONDS` until + # the deadline. Hot, and bounded, and it still terminates — the right + # trade against exiting mid-critical-section. + _roster_deadline=$((SECONDS + ROSTER_SYNC_BUDGET_S)) + while [ ! -s "$_roster_sentinel" ] && [ "$SECONDS" -lt "$_roster_deadline" ]; do + sleep 1 || true + done + fi + + if [ -s "$_roster_sentinel" ]; then + _roster_status="$(cat "$_roster_sentinel" 2>/dev/null || printf '13')" + wait "$_roster_child" 2>/dev/null || true + rm -f "$_roster_sentinel" "$_roster_pidfile" || true + [ "$_roster_status" = "0" ] || exit "$_roster_status" + else + # No sentinel: the wrapper has not reached its last act. Stop it, give it a + # moment, insist, and REAP it — the lock is not released while the process + # that was writing under it may still be running, because letting the next + # caller in beside a live writer is worse than the leak being fixed. + _roster_inner="$(cat "$_roster_pidfile" 2>/dev/null || true)" + # A leading zero, a negative, a word, or an empty file all mean the same + # thing here: there is no pid to work with. Cleared once, so the predicate + # refuses it in one place rather than every caller guessing. + case "$_roster_inner" in + ''|*[!0-9]*|0*) _roster_inner="" ;; + esac + # WHAT WAS ACTUALLY ATTEMPTED IS RECORDED, because the state can change + # under us between one signal and the next (raised in review, and it stood + # for three rounds before I read it properly). "Attempted" is the whole + # claim — see the note below, and do not widen this heading back to + # "sent": the block would then state two different contracts. + # + # The predicate is re-asked before TERM and again before KILL, which is + # right — but it means a pid that was `ours` at the first can be `unknown` + # at the second, through recycling or an argv that stopped being readable. + # The diagnostic then said "nothing was signalled on that number", which + # would be FALSE: TERM had already gone out. An operator reading that + # would go looking for a process nobody had touched. + # ATTEMPTED, and the name is the claim. `kill ... || true` discards its + # status, so what this shell can honestly record is that it CALLED kill — + # not that a signal was accepted, and certainly not that one was + # delivered. An earlier version set these beside the call and then the + # diagnostic said "TERM WAS SENT", which under EPERM or ESRCH is false: + # the same "discarded the result of kill" boundary this path exists to + # respect, crossed again in the reporting (raised in review). + _roster_term_attempted=0 + _roster_kill_attempted=0 + # Signalled ONLY when it is still the process we started. Anything else is + # left alone: the cost of not signalling our own child is a lock we keep + # and report; the cost of signalling someone else's is a process we had no + # business touching. + if _roster_inner_still_ours; then + kill -TERM "$_roster_inner" 2>/dev/null || true + _roster_term_attempted=1 + fi + # The wrapper needs no such check — it is this shell's own child, held in + # `$!` and not reaped until below, so its number cannot have been recycled + # while we still hold it. + kill -TERM "$_roster_child" 2>/dev/null || true + # `|| true`: a failed spawn here would exit before the KILL and the reap, + # leaving the EXIT trap to release the lock beside a child that ignored + # TERM — the precise case this grace period exists for. + sleep 2 || true + # Re-asked, not remembered: the grace period is exactly the window in + # which our child can exit and its number be handed to someone else. A + # KILL aimed at an answer obtained two seconds ago is the same defect as + # the TERM above, one branch later. + # ESCALATION IS MONOTONE, and this is a safety rule rather than a tidiness + # one (raised in review). The predicate is re-asked, so the other + # direction of the transition is possible too: `unknown` before TERM, then + # `ours` before KILL, through recycling or an instrument that failed once + # and recovered. The old code would then send KILL to a number it had just + # refused to send TERM to — escalating against something it never + # established a claim on. + # + # So KILL requires BOTH: that we were entitled to TERM this number, and + # that it is still ours now. A number once judged unidentifiable is not + # re-adopted later in the same timeout. + if [ "$_roster_term_attempted" = "1" ] && _roster_inner_still_ours; then + kill -KILL "$_roster_inner" 2>/dev/null || true + _roster_kill_attempted=1 + fi + kill -KILL "$_roster_child" 2>/dev/null || true + wait "$_roster_child" 2>/dev/null || true + # Read once more AFTER the reap: a wrapper that was finishing while this + # side gave up would otherwise be reported as a timeout it never had. + # + # THE SENTINEL DOES NOT SHORT-CIRCUIT THE GATE (raised in review). It is + # written by the wrapper only after `wait` on node returned, so it *should* + # imply the inner process is gone — but that is an argument about the + # wrapper's code, not an observation of this machine, and the branch it + # guards is the one that releases the lock. So the same question is asked + # here too. It costs nothing when the inner really has exited: the + # predicate fails on its first call and this reads exactly as it did + # before. + # AND IT ASKS FOR THE WORD, NOT FOR A YES/NO. Gating this on + # `_roster_inner_still_ours` was the same collapse one layer up: "not + # ours" released the lock, and "not ours" includes "alive, unidentifiable" + # (raised in review). Only `gone` may pass. + if [ -s "$_roster_sentinel" ] && [ "$(_roster_inner_state)" != "gone" ]; then + AGMSG_HELD_LOCKS="" + rm -f "$_roster_sentinel" "$_roster_pidfile" || true + echo "agmsg: roster sync $operation failed for team '$team': the wrapper recorded an exit status, but process '$_roster_inner' could not be confirmed to have exited (state: $(_roster_inner_state)). The team lock is being KEPT rather than released. Check that process, then remove $team_dir/.config.lock" >&2 + exit 17 + fi + if [ -s "$_roster_sentinel" ]; then + _roster_status="$(cat "$_roster_sentinel" 2>/dev/null || printf '13')" + rm -f "$_roster_sentinel" "$_roster_pidfile" || true + [ "$_roster_status" = "0" ] || exit "$_roster_status" + else + # "I SENT A SIGNAL" IS NOT "THE PROCESS IS GONE" (raised as BLOCKING). + # + # Everything above discards its result: `kill -TERM ... || true`, then + # `kill -KILL ... || true`. Neither says whether anything died. The inner + # process is a GRANDCHILD, so `wait` cannot speak for it either — only + # the wrapper is this shell's child. So the release below used to rest on + # nothing at all: a signal refused by a sandbox, or a process wedged in an + # uninterruptible state, and the lock came off anyway with the writer + # still running. That is the one outcome this whole path exists to avoid, + # and the comment two branches up says so in as many words. + # + # So the question is ASKED, with the repo's own instrument. + # `_agmsg_pid_alive_local` is the right one of the two: this pid was + # minted by `$!` in one of these shells and read back from a pidfile one + # of them wrote. It reads EPERM as alive, treats a zombie as gone, and + # cross-checks with `ps` so a sandbox that refuses to signal cannot be + # mistaken for a process that is not there. + # + # AND IT ASKS THE SAME QUESTION THE SIGNALS DID, not a weaker one. An + # earlier version gated on liveness alone, which reads a recycled number + # as "our child is still running" (raised in review). The predicate is + # the pair — alive AND still carrying this operation's argv — so the two + # cannot drift apart. + # + # WHICH WAY IT FAILS MATTERS MORE THAN WHETHER IT CAN: + # + # wrong "still ours" the lock is held when it need not be — visible, + # named, and fixable by hand. + # wrong "gone" the lock comes off beside a live writer. Silent, + # and it corrupts. + # + # `_agmsg_pid_alive_local` errs toward "alive"; the argv match errs + # toward "not ours". Their conjunction therefore errs toward "gone" — + # the WRONG direction — so a pid that is alive but unrecognisable is + # reported as UNKNOWN below rather than folded into either answer. + # Waited on only while the answer is `ours` — the one state in which the + # process may still be finishing. `gone` and `unknown` are both terminal + # here: nothing about waiting turns an unreadable argv into a readable + # one. + _roster_confirm_deadline=$((SECONDS + 5)) + while [ "$(_roster_inner_state)" = "ours" ] && [ "$SECONDS" -lt "$_roster_confirm_deadline" ]; do + # `|| true` for the third time, and for the same contract: exiting + # here would release the lock through the trap without ever reaching + # the decision below. + sleep 1 || true + done + + # ONE READ OF THE WORD, THEN THREE ARMS. Taken once rather than per + # branch so the arms cannot disagree about what was observed. + _roster_state="$(_roster_inner_state)" + + # THE DIAGNOSTIC REPORTS WHAT WAS ATTEMPTED, once, for both exits below. + # Computed here rather than in each branch so the two cannot describe + # the same run differently — and worded as ATTEMPTS, because `kill`'s + # status was discarded and delivery is not observable from this side at + # all. + _roster_signal_note="No signal was attempted on that number: it was never identifiable as this operation's child" + if [ "$_roster_term_attempted" = "1" ] && [ "$_roster_kill_attempted" = "1" ]; then + _roster_signal_note="TERM and then KILL were both ATTEMPTED on that number while it carried this operation's argv; whether either was accepted or delivered is not known here" + elif [ "$_roster_term_attempted" = "1" ]; then + _roster_signal_note="TERM was ATTEMPTED on that number while it carried this operation's argv, and KILL was then WITHHELD because it no longer did — so a process may have been signalled once and not stopped" + fi + + if [ "$_roster_state" = "unknown" ]; then + AGMSG_HELD_LOCKS="" + rm -f "$_roster_sentinel" "$_roster_pidfile" || true + echo "agmsg: roster sync $operation failed for team '$team': the local roster child did not finish within ${ROSTER_SYNC_BUDGET_S}s, and its fate could not be established — the recorded pid was '$_roster_inner', which is either absent or alive without this operation's argv. $_roster_signal_note. The team lock is being KEPT rather than released, since releasing it beside a writer that may still be running can corrupt the roster journal. Check that process, then remove $team_dir/.config.lock" >&2 + exit 18 + fi + + if [ "$_roster_state" = "ours" ]; then + # THE LOCK IS KEPT, ON PURPOSE, AND THE OPERATOR IS TOLD. + # + # Emptying `AGMSG_HELD_LOCKS` is how the lock survives: the EXIT trap + # calls `agmsg_lock_release`, which walks that list and does nothing + # when it is empty. The directory stays where it is. Reaching for the + # trap itself would fight the library that installed it; this uses the + # library's own state. + # + # This does not reopen #821. That property is "a child that hangs must + # not hold the critical section INDEFINITELY, silently" — and this + # shell exits, now, with the pid and the path to remove. What it + # refuses to do is claim a process was stopped when it was not. + AGMSG_HELD_LOCKS="" + rm -f "$_roster_sentinel" "$_roster_pidfile" || true + echo "agmsg: roster sync $operation failed for team '$team': the local roster child did not finish within ${ROSTER_SYNC_BUDGET_S}s, and process $_roster_inner is STILL RUNNING and still carrying this operation's argv. $_roster_signal_note. The team lock is being KEPT rather than released, because releasing it beside a live writer can corrupt the roster journal. Stop that process, then remove $team_dir/.config.lock" >&2 + exit 17 + fi + + # ONLY `gone` REACHES HERE, AND IT IS SAID RATHER THAN LEFT IMPLIED. + # The two arms above return, so falling through means the word was + # `gone` — but a fourth state added later would fall through too, and + # inherit a release nobody meant to give it. This refuses instead. + if [ "$_roster_state" != "gone" ]; then + AGMSG_HELD_LOCKS="" + rm -f "$_roster_sentinel" "$_roster_pidfile" || true + echo "agmsg: roster sync $operation failed for team '$team': internal error — the roster child's state was reported as '$_roster_state', which this code does not know how to act on. The team lock is being KEPT rather than released. Remove $team_dir/.config.lock once you are satisfied nothing is writing to the team" >&2 + exit 19 + fi + rm -f "$_roster_sentinel" "$_roster_pidfile" || true + # Released here rather than left to the EXIT trap. The trap is the + # mechanism that is not reached when a child never returns, and a fix + # that leans on it on the one path it exists for is not a fix. It still + # runs afterwards and finds nothing to do. + agmsg_lock_release + echo "agmsg: roster sync $operation failed for team '$team': the local roster child did not finish within ${ROSTER_SYNC_BUDGET_S}s; it was stopped, confirmed gone, and the team lock released" >&2 + # Stopping between the journal write and the state write leaves the two + # out of step. Each is written by rename, so neither is torn — but that + # the next run converges from every such point is NOT established, and is + # recorded on the issue rather than claimed here. + exit 14 + fi + fi +fi + case "$operation" in reconcile|apply) agmsg_roster_project_config "$team_dir" "$config" ;; esac diff --git a/scripts/key.sh b/scripts/key.sh index 1d2e55448..0ea8d727d 100755 --- a/scripts/key.sh +++ b/scripts/key.sh @@ -35,6 +35,10 @@ source "$SCRIPT_DIR/lib/operator-guidance.sh" source "$SCRIPT_DIR/lib/shquote.sh" # shellcheck disable=SC1091 source "$SCRIPT_DIR/lib/roster-journal.sh" +# agmsg_sha256 -- `shasum` is absent in Git for Windows' Git Bash, and every +# digest below is either a fingerprint a human compares or an E2EE checkpoint. +# shellcheck source=lib/hash.sh +source "$SCRIPT_DIR/lib/hash.sh" TEAMS_DIR="$CONNECTION_ROOT/teams" CRED_ROOT="$CONNECTION_ROOT/run/remote-credentials" @@ -86,12 +90,25 @@ _key_read_config_field() { # Short, human-comparable digest of a recipient string (SSH-key-fingerprint # style grouping) — for the H7 fingerprint-verification step: # two people compare this same short string over a separate channel. +# +# Fails rather than returning a short string it could not compute: see the +# callers, which now take the value into a variable of its own before printing +# it. An empty fingerprint is the worst possible output here -- both people see +# the same blank and agree. +# +# THAT REFUSAL RIDES ON `set -o pipefail`, LINE 2. `agmsg_sha256` is in the +# middle of this pipeline, and `cut` and `sed` are perfectly happy with the +# empty input a failed digest leaves them: without pipefail the pipeline exits 0 +# with an empty string, the caller's assignment succeeds, and the label prints +# with nothing after it. Dropping `pipefail` reddens both "no blank +# fingerprint" cases in tests/test_key.bats, which is the control for this +# paragraph -- if you are here because you want to simplify line 2, run them. _key_fingerprint() { - printf '%s' "$1" | shasum -a 256 | cut -c1-16 | sed 's/\(....\)/\1-/g;s/-$//' + printf '%s' "$1" | agmsg_sha256 | cut -c1-16 | sed 's/\(....\)/\1-/g;s/-$//' } _key_fingerprint_sha256() { - printf '%s' "$1" | shasum -a 256 | awk '{print $1}' + printf '%s' "$1" | agmsg_sha256 } # A timestamp alone collides when two epochs are minted within the same @@ -295,8 +312,10 @@ cmd_generate() { existing="$(_key_read_config_field "$cfg" '$.remote_key.current.key_id')" if [ -n "$existing" ] && [ "$existing" != "null" ]; then agmsg_lock_release - echo "agmsg: team '$team' already has a key (key_id=$existing); rotation is not available in this release. To view it:" >&2 + echo "agmsg: team '$team' already has a key (key_id=$existing). To view it:" >&2 echo " bash $(agmsg_shq "$SKILL_DIR/scripts/key.sh") show $(agmsg_shq "$team")" >&2 + echo "To mint a replacement epoch instead:" >&2 + echo " bash $(agmsg_shq "$SKILL_DIR/scripts/key.sh") rotate $(agmsg_shq "$team")" >&2 exit 1 fi @@ -319,8 +338,17 @@ cmd_generate() { _key_write_epoch_locked "$cfg" "$(_key_epoch_json "$key_id" 0 0 "$recipient" null "$created_at")" agmsg_lock_release + # Computed into a variable of its own, NOT inline in the echo. A command + # substitution that fails inside a simple command's arguments leaves that + # command's own status untouched, so `echo` succeeded and printed the label + # with nothing after it. A bare assignment's status IS the substitution's, so + # `set -e` stops here instead. (Same reasoning as remote.sh's `existing=` note; + # `local fp_short="$(...)"` would put the status back on the declaration and + # undo it.) + local fp_short + fp_short="$(_key_fingerprint "$recipient")" echo "Generated a new key for team '$team'." - echo "Recipient fingerprint: $(_key_fingerprint "$recipient")" + echo "Recipient fingerprint: $fp_short" echo # What the key IS, always: true whoever ran this, so it is never held back. # @@ -413,8 +441,10 @@ cmd_show() { fi if [ "$reveal" -eq 0 ]; then + local fp_short + fp_short="$(_key_fingerprint "$recipient")" echo "Team: $team" - echo "Recipient fingerprint: $(_key_fingerprint "$recipient")" + echo "Recipient fingerprint: $fp_short" echo "Public recipient: $recipient" return fi @@ -576,8 +606,10 @@ cmd_import() { _key_write_identity_atomic "$cred_dir/$staged_key_id.key" "$identity" agmsg_lock_release unset identity + local fp_short + fp_short="$(_key_fingerprint "$recipient")" echo "Imported replacement key for team '$team' (key_id=$staged_key_id)." - echo "Recipient fingerprint: $(_key_fingerprint "$recipient")" + echo "Recipient fingerprint: $fp_short" return fi # Matches the existing epoch: just store this device's copy of the @@ -596,8 +628,10 @@ cmd_import() { fi unset identity + local fp_short + fp_short="$(_key_fingerprint "$recipient")" echo "Imported key for team '$team'." - echo "Recipient fingerprint: $(_key_fingerprint "$recipient")" + echo "Recipient fingerprint: $fp_short" } cmd_rotate() { @@ -760,7 +794,7 @@ EOF echo "agmsg: could not read the current authority-confirmed epoch snapshot." >&2 exit 1 fi - previous_snapshot_sha="$(shasum -a 256 "$previous_snapshot" | awk '{print $1}')" + previous_snapshot_sha="$(agmsg_sha256 < "$previous_snapshot")" rm -f "$previous_snapshot" writer_generation="$(agmsg_sqlite_mem \ "SELECT CAST('$(_agmsg_sqlesc "$(_key_read_config_field "$cfg" '$.remote_key.current.writer_generation')")' AS INTEGER) + 1;")" @@ -781,8 +815,10 @@ EOF fi agmsg_lock_release + local fp_short + fp_short="$(_key_fingerprint "$recipient")" echo "Generated replacement key for team '$team' (epoch=$next_epoch, key_id=$key_id)." - echo "Recipient fingerprint: $(_key_fingerprint "$recipient")" + echo "Recipient fingerprint: $fp_short" echo "The private key was not written to the journal; distribute it out of band." echo "On an interactive terminal, run:" echo " bash $(agmsg_shq "$SKILL_DIR/scripts/key.sh") show $(agmsg_shq "$team") --key-id $(agmsg_shq "$key_id") --reveal-secret" diff --git a/scripts/lib/compat.sh b/scripts/lib/compat.sh index 99c92370b..883ed04f3 100644 --- a/scripts/lib/compat.sh +++ b/scripts/lib/compat.sh @@ -150,7 +150,14 @@ compat_get_comm() { fi ;; *) - ps -o comm= -p "$pid" 2>/dev/null | xargs basename 2>/dev/null + # `ps -o comm=` prints the executable path on macOS. Piping it through + # `xargs basename` splits that path on whitespace (and eats quotes), so a + # binary under e.g. "~/Library/Application Support/..." resolves to + # "Application". Take the basename of the whole string instead. + local _comm + _comm=$(ps -o comm= -p "$pid" 2>/dev/null) || return 1 + [ -n "$_comm" ] || return 1 + basename -- "$_comm" 2>/dev/null ;; esac } diff --git a/scripts/lib/hash.sh b/scripts/lib/hash.sh index 65367092a..d11b0f798 100644 --- a/scripts/lib/hash.sh +++ b/scripts/lib/hash.sh @@ -27,3 +27,170 @@ agmsg_sha1() { cksum | awk '{print $1}' fi } + +# Portable SHA-256 of stdin, emitting the bare hex digest. +# +# Same absence as above -- `shasum` is not in Git for Windows' Git Bash -- and +# the same fixed order, so a given machine always answers with one tool: +# shasum -a 256 (macOS/Linux) -> sha256sum (Git Bash/Linux) -> openssl. +# +# THE LAST RESORT IS DIFFERENT ON PURPOSE, and it is the point of this helper. +# agmsg_sha1 ends in `cksum` because its callers name a socket after the digest +# and need only that the same input give the same name on the same machine. +# These callers need the opposite property. The digest is +# +# * the fingerprint two people read to each other over a separate channel to +# confirm they are talking to the key they think they are, and +# * the age-v1 checkpoint that says an epoch snapshot is the snapshot it +# claims to be. +# +# A non-cryptographic stand-in does not weaken those, it makes them say +# something untrue. So when no tool here can compute SHA-256 this FAILS, and +# every caller is expected to stop rather than carry an empty or substitute +# value forward. +# +# Do not add a `cksum` arm to "make this consistent with agmsg_sha1". The +# inconsistency is the decision. +# +# THE FAILURE IS THE HELPER'S OWN, not the caller's shell options. Written as +# `shasum -a 256 | awk …` this function's status was the status of `awk`, which +# is delighted by the empty input a failed digest hands it — so "this FAILS" +# held only because `key.sh` and `remote.sh` happen to `set -o pipefail` on +# their second line. A caller without it got an empty success and carried it +# into a fingerprint. Each tool is now run as its own command substitution with +# its status checked here, so the refusal travels with the function. +# +# WHICH ARM RUNS IS DECIDED BY PRESENCE, AND A CHOSEN ARM THAT FAILS IS THE END. +# The fallback exists for a tool that is ABSENT, not for one that is broken: +# there is no second attempt after `shasum` is found and then fails. That is +# deliberate — a machine whose `shasum` is broken has something wrong with it +# that a quiet substitution would hide — and it is asserted below, so changing +# it has to be a decision rather than a drift. +# +# The answer is then checked for being 64 lowercase hex. A tool that exits 0 and +# prints a warning, a path, or an empty line has not failed as far as `$?` is +# concerned, and this value is not a label: it is what two people read to each +# other to confirm a key, and what the age-v1 checkpoint pins a snapshot with. +# Lowercase specifically, because all three arms emit lowercase and a fourth +# that did not would leave two machines disagreeing about a fingerprint that is +# "the same". +# +# The two checks overlap on purpose and are not redundant: a tool that exits +# non-zero while still printing a well-formed digest is caught only by the +# status, and one that exits zero while printing anything else only by the +# shape. There is a case for each below. +# The arms and the shape check, without the self-test below -- so the self-test +# can call it without calling itself. +_agmsg_sha256_selected() { + local raw + if command -v shasum >/dev/null 2>&1; then + raw="$(shasum -a 256)" || return 1 + raw="${raw%% *}" + elif command -v sha256sum >/dev/null 2>&1; then + raw="$(sha256sum)" || return 1 + raw="${raw%% *}" + elif command -v openssl >/dev/null 2>&1; then + raw="$(openssl dgst -sha256)" || return 1 + raw="${raw##* }" + else + echo "agmsg: no SHA-256 tool found on PATH (looked for shasum, sha256sum, openssl)." >&2 + echo "One of these is required for key fingerprints and end-to-end-encryption checkpoints." >&2 + return 1 + fi + case "$raw" in + *[!0-9a-f]*|'') + echo "agmsg: the SHA-256 tool on PATH answered with something that is not a digest." >&2 + return 1 + ;; + esac + [ "${#raw}" -eq 64 ] || { + echo "agmsg: the SHA-256 tool on PATH answered with ${#raw} characters, not a 64-hex digest." >&2 + return 1 + } + printf '%s\n' "$raw" +} + +# ASK THE SELECTED TOOL A QUESTION WE KNOW THE ANSWER TO, BEFORE EVERY DIGEST. +# +# `_agmsg_sha256_selected` accepts any 64 lowercase hex, which is the shape of +# a digest and not the proof of one: a tool that exits 0 and prints a plausible +# but wrong value is accepted, and that value then becomes a fingerprint two +# people read to each other, or the checkpoint that says a snapshot is the +# snapshot it claims to be. +# +# The check lives HERE rather than at the callers because the alternative is a +# list of entry points that must be kept complete -- `key.sh` is its own CLI and +# `generate`, `show`, `import` and `rotate` all reach a digest without going +# anywhere near `connect`'s preflight. A list like that is exactly what was +# already missed once. +# +# RUN BEFORE EVERY DIGEST, AND NOT MEMOISED. It was, on one head, keyed on a +# shell variable -- which review took apart twice over. The flag was read +# straight from the environment, so `_AGMSG_SHA256_VERIFIED=1` in a preseeded +# environment meant "already checked" and skipped the check outright: an +# undocumented env override that turned a fail-closed contract off. And it did +# not even work: every production call is `printf | agmsg_sha256` or +# `x="$(agmsg_sha256 …)"`, both subshells, so the flag never reached the parent +# and the self-test ran again anyway. The saving was imaginary and the hole was +# not. +# +# So the cost is stated instead of avoided: one extra digest of a 5-byte input +# per digest taken. The command that takes the most is `key rotate` with an +# accepted rotation to check -- the accepted recipient's fingerprint, the new +# recipient's journal fingerprint, the previous snapshot, and the short +# fingerprint printed at the end: FOUR digests, so eight runs of the tool. Every +# one of them sits beside file and lock work that dwarfs it. +_agmsg_sha256_selftest() { + local probe + probe="$(printf '%s' probe | _agmsg_sha256_selected)" || return 1 + if [ "$probe" != 'ba9c736f19e7f60b7f6764adb0b7908c0a2b394e09b6c09863528c7f2bc86095' ]; then + echo "agmsg: the SHA-256 tool on PATH returned the wrong digest for a known input." >&2 + echo "Its answers cannot be used for key fingerprints or encryption checkpoints." >&2 + return 1 + fi +} + +agmsg_sha256() { + _agmsg_sha256_selftest || return 1 + _agmsg_sha256_selected +} + +# True when agmsg_sha256 has something to run. Separated so a caller can ASK +# before it starts, rather than discover it at the digest. +# +# The order matters more than it looks: the first SHA-256 in a `connect --e2ee` +# comes AFTER the team has been registered with the server, so without this the +# operator's first news of a missing tool is a half-finished connect. Same +# category as the `age` check next to it -- a prerequisite of end-to-end +# encryption, not of agmsg -- and asked at the same moment. +# Probed by RUNNING it, not by `command -v`. The question is whether this +# machine can produce a SHA-256, and presence on PATH is only a proxy for that: +# a tool that is installed and fails answers "yes" to the proxy and "no" to the +# question, which is the direction that hurts -- the preflight passes and the +# digest fails later, which is the shape of #861 all over again. Costs two runs +# of the tool -- `agmsg_sha256`'s self-test and this probe's own digest -- on a +# command that is about to make a network round trip. +# +# Nothing more than "can this machine produce one", because the correctness +# question moved into `agmsg_sha256` itself -- a preflight that knows something +# the digest path does not is the shape of #861, and for one head this function +# was the only thing checking the answer while `key.sh` reached a digest by four +# routes that never call it. +agmsg_sha256_usable() { + printf '%s' probe | agmsg_sha256 >/dev/null 2>&1 +} + +# Refuse to proceed without one, with the same install guidance shape the age +# preflight uses. +agmsg_require_sha256() { + if ! agmsg_sha256_usable; then + echo "agmsg: end-to-end encryption needs a working SHA-256 tool, and this device has none." >&2 + echo "One may be installed: a tool that is present but fails, or answers a known input" >&2 + echo "wrongly, is reported here the same way as one that is absent -- neither can be used." >&2 + echo "agmsg looks for 'shasum', then 'sha256sum', then 'openssl'. Install or repair one:" >&2 + echo " macOS (Homebrew): brew install openssl" >&2 + echo " Debian/Ubuntu: sudo apt install coreutils" >&2 + echo " Windows (Git Bash): ships with Git for Windows; reinstall it if 'sha256sum' is missing" >&2 + return 1 + fi +} diff --git a/scripts/lib/resolve-project.sh b/scripts/lib/resolve-project.sh index e25865720..58a64b2f2 100644 --- a/scripts/lib/resolve-project.sh +++ b/scripts/lib/resolve-project.sh @@ -228,6 +228,9 @@ agmsg_find_registered_project_variant() { } # Map an agent type to the binary basename(s) its process may carry. +# Names must be type-distinctive. Do not list `agent`: Homebrew grok-build +# and the Cursor CLI installer both use that basename (#856). Matching it +# would attach the wrong pid (#93). The alias is an intentional miss. _agmsg_agent_binaries() { case "$1" in claude-code) echo "claude" ;; @@ -236,6 +239,7 @@ _agmsg_agent_binaries() { antigravity) echo "antigravity" ;; copilot) echo "copilot" ;; opencode) echo "opencode" ;; + grok-build) echo "grok" ;; *) echo "claude codex gemini" ;; esac } diff --git a/scripts/lib/roster-journal.sh b/scripts/lib/roster-journal.sh index 29a7a78d5..eb1887901 100644 --- a/scripts/lib/roster-journal.sh +++ b/scripts/lib/roster-journal.sh @@ -386,3 +386,76 @@ agmsg_roster_project_config() { fi agmsg_write_atomic "$config" "$updated" } + +# agmsg_roster_argv_is_ours +# +# "Is this command line the roster-sync run I started?" — the whole question, +# in one place, callable. +# +# IT LIVES HERE SO IT CAN BE DRIVEN DIRECTLY. It began inside +# `roster-sync-driver.sh`, where the only way to reach it was to run the +# driver — so its cases ended up re-implementing the comparison in the test +# file and asserting on the driver with `grep`. That measures a copy and the +# spelling of the original, never the original itself (raised in review). +# +# The answer must be conservative in ONE direction. A false "yes" sends TERM +# and KILL to a process that is not ours; a false "no" leaves a lock held and +# reported. So every widening below is deliberate and bounded, and anything +# unrecognised is "no". +agmsg_roster_argv_is_ours() { + local cmdline="$1" script="$2" operation="$3" config="$4" + [ -n "$cmdline" ] && [ -n "$script" ] && [ -n "$operation" ] && [ -n "$config" ] || return 1 + + # QUOTES ARE A BOUNDARY, NOT DECORATION — AND DELETING THEM DESTROYS ONE + # (raised in review, after an earlier version of this did exactly that). + # + # A native Windows command line quotes any argument containing a space, so + # `compat_get_cmdline` can return + # "C:/Users/First Last/.../roster-sync.mjs" reconcile "C:/.../config.json" + # and an unquoted needle never matches it. The first fix stripped every `"` + # from the haystack. That is worse than it looks: it is true that the space + # between two arguments survives, but a quote also carries "the spaces INSIDE + # me are not argument separators". Strip it and + # node other.js --note "