From bc77f80e67358c1d5cc32a2a9e86106f37cd98ff Mon Sep 17 00:00:00 2001 From: fujibee Date: Sat, 15 Aug 2026 01:57:48 -0700 Subject: [PATCH 01/18] ci(windows): a feed outage must not be reported as this change's red (#824) choco install sqlite -y --no-progress was one call with no retry, no cache and -- the part that did the damage -- no check. Measured on run 31869238895 (install helpers): steps 4, 5, 6 and 7 all success, step 8 failure. The install printed 'Chocolatey installed 0/0 packages' after a 503 from the community feed and EXITED ZERO; the version step's || echo turned the missing binary into a success line; the red landed three steps later on Run tests. Reading that check list, the failure belongs to the change under test. It did not. Worse than intermittent: the same outage decides a DIFFERENT step's colour depending on where chocolatey's two-phase resolution fails. Run 31870847862 on another branch got 499 on the IsLatestVersion query, was counted 0/1 with 1 failed, and exited non-zero -- so the install step went red there. Same cause, two different reds, neither of them reproducible in place. Three changes, one per problem: cache, keyed on a pinned version -- most runs never ask the feed retry, three attempts widening -- the failures are per-request, not an outage (both Windows legs run concurrently and on 31869238895 one installed fine while the other did not) a dedicated presence check that is the only step allowed to be red about this, named so the check list says chocolatey rather than tests Also drops the || echo fallback from Show tool versions. Absence is now decided by a step that may fail; that one only reports. The install step deliberately exits 0 after its retries: whether one choco invocation worked is a different question from whether the dependency is present, and only the second may decide the job's colour. --- .github/workflows/tests.yml | 89 +++++++++++++++++++++++++++++++++++-- 1 file changed, 85 insertions(+), 4 deletions(-) diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index 04e590d2b..854fe4267 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -58,6 +58,11 @@ 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 + # Pinned so the Windows sqlite cache key means something: a hit is the same + # binary, not whatever the feed is serving today (#824). Bumping this is one + # cache miss and one trip to chocolatey, which is the only time the community + # feed is asked at all. + WINDOWS_SQLITE_VERSION: '3.53.4' jobs: # Cheap gate: is this PR's diff entirely documentation? Pushes to main always @@ -214,7 +219,23 @@ jobs: if: needs.changes.outputs.docs_only != 'true' run: | bash --version | head -1 - sqlite3 --version + # Two changes here, and they pull 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 now decided by the presence check above, which is + # allowed to be red about it. + # + # But 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 that drives 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, not external input. + if [ "${{ matrix.sqlite }}" = "true" ]; then + sqlite3 --version + fi bats --version # Diagnostics for the shards that report every test ok and then sit @@ -575,15 +596,71 @@ jobs: if: needs.changes.outputs.docs_only == 'true' run: echo "Diff is documentation-only; skipping the Windows bats legs." - - name: Install sqlite3 + # A THIRD PARTY'S BAD MINUTE MUST NOT BE REPORTED AS THIS CHANGE'S RED + # (#824). `choco install sqlite -y --no-progress` was one call with no + # retry, no cache and — the part that did the damage — no check. On a 503 + # from the community feed it printed "Chocolatey installed 0/0 packages" + # and EXITED ZERO, so the step went green, the version step's + # `|| echo "not found"` turned a missing binary into a success line, and + # the red landed three steps later on `Run tests`. Measured on + # run 31869238895: steps 4, 5, 6 and 7 all `success`, step 8 `failure`, + # with `sqlite3: command not found` in step 7's own log. + # + # Reading that check list, the failure belongs to the change under test. + # It did not. The feed's flakiness is not ours to fix; which step goes red + # is. + # + # EVERY STEP HERE IS GATED ON `matrix.sqlite`, which #822 added. A leg that + # never opens a store must not be stopped by a package feed either — that + # is the contract #822 built, and hardening the install would have + # cancelled it by making `driver input (#817)` depend on chocolatey again. + # + # The restore is tried first so most runs never ask the feed at all: the + # key is the pinned version, so a hit is the same binary and a miss is the + # only thing that needs the network. + - name: Restore sqlite3 (cache) if: needs.changes.outputs.docs_only != 'true' && matrix.sqlite + id: sqlite-cache + uses: actions/cache@v4 + with: + path: | + C:\ProgramData\chocolatey\lib\SQLite + C:\ProgramData\chocolatey\bin\sqlite3.exe + key: sqlite3-${{ runner.os }}-choco-${{ env.WINDOWS_SQLITE_VERSION }} + + - name: Install sqlite3 + 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: | + # Three attempts with a widening pause. The observed failures are 503 + # and 504 from `community.chocolatey.org`, and they are per-request + # rather than an outage: on run 31869238895 the two Windows legs ran + # concurrently and one of them installed fine. + for ($attempt = 1; $attempt -le 3; $attempt++) { + choco install sqlite --version=$env:WINDOWS_SQLITE_VERSION -y --no-progress + if (Test-Path 'C:\ProgramData\chocolatey\bin\sqlite3.exe') { exit 0 } + Write-Host "::warning::sqlite3 not installed on attempt $attempt; the chocolatey feed is answering errors" + Start-Sleep -Seconds (10 * $attempt) + } + exit 0 # the hard check is its own step, so a retry loop cannot decide the colour - name: Put chocolatey shims on PATH (Git Bash form) if: needs.changes.outputs.docs_only != 'true' && matrix.sqlite run: echo "/c/ProgramData/chocolatey/bin" >> "$GITHUB_PATH" + # THE STEP THAT IS ALLOWED TO BE RED ABOUT THIS. Separate from the install + # on purpose: whether the dependency is present is a different question + # from whether one `choco` invocation worked, and only this one may decide + # the job's colour. Named so the check list says what is wrong — a reader + # seeing this red does not go looking through the diff. + - name: sqlite3 must be present, or this leg is about chocolatey + if: needs.changes.outputs.docs_only != 'true' && matrix.sqlite + run: | + if ! sqlite3 --version; then + echo "::error::sqlite3 is missing. The chocolatey community feed did not serve it (503/504 seen on 2026-08-15). 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' run: | @@ -595,7 +672,11 @@ jobs: if: needs.changes.outputs.docs_only != 'true' run: | bash --version | head -1 - sqlite3 --version || sqlite3.exe --version || echo "sqlite3 not found on PATH" + # No `|| echo` fallback (#824): it turned a missing binary into a + # success line, so nothing between the failed install and `Run tests` + # said anything was wrong. Absence is decided by the step above, which + # is allowed to be red about it; this one only reports. + sqlite3 --version bats --version - name: Run tests From 9bbb7a7e4a6cd4e7ce256e235ea2bc474fbc70ed Mon Sep 17 00:00:00 2001 From: fujibee Date: Sat, 15 Aug 2026 02:00:45 -0700 Subject: [PATCH 02/18] PROBE (reverted next commit): point the pin at a version the feed cannot serve Stop condition 2 asks to see the CI behave correctly while the dependency cannot be fetched. The feed recovered before this change was written, so the condition is created rather than waited for: a nonexistent version busts the cache key AND makes the install fail, which is the same path a 503 takes. What is being observed is which step goes red. Expected: Install sqlite3 success (exits 0 after its retries) sqlite3 must be present, ... failure <- only this one Run tests skipped Today the same outage reddens either Install sqlite3 or Run tests depending on which phase of chocolatey's resolution fails. This commit exists to show that is now fixed, and it is reverted immediately after the observation. --- .github/workflows/tests.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index 854fe4267..8cc487582 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -62,7 +62,7 @@ env: # binary, not whatever the feed is serving today (#824). Bumping this is one # cache miss and one trip to chocolatey, which is the only time the community # feed is asked at all. - WINDOWS_SQLITE_VERSION: '3.53.4' + WINDOWS_SQLITE_VERSION: '0.0.0-probe-824' # PROBE: reverted in the next commit jobs: # Cheap gate: is this PR's diff entirely documentation? Pushes to main always From 8b0072dfbf656578f8c174d19bc4c73b942589c4 Mon Sep 17 00:00:00 2001 From: fujibee Date: Sat, 15 Aug 2026 02:04:43 -0700 Subject: [PATCH 03/18] Revert "PROBE (reverted next commit): point the pin at a version the feed cannot serve" This reverts commit 41fd8aed0400162c7a4f16d010df9d97e7eec69a. --- .github/workflows/tests.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index 8cc487582..854fe4267 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -62,7 +62,7 @@ env: # binary, not whatever the feed is serving today (#824). Bumping this is one # cache miss and one trip to chocolatey, which is the only time the community # feed is asked at all. - WINDOWS_SQLITE_VERSION: '0.0.0-probe-824' # PROBE: reverted in the next commit + WINDOWS_SQLITE_VERSION: '3.53.4' jobs: # Cheap gate: is this PR's diff entirely documentation? Pushes to main always From 29b9765b3cc01402f084473756d5262f548b9a3e Mon Sep 17 00:00:00 2001 From: fujibee Date: Sat, 15 Aug 2026 02:34:30 -0700 Subject: [PATCH 04/18] PROBE 2 (reverted next commit): the composed control, on top of #822 The first probe ran before this branch was rebased onto #822, so both Windows legs it could observe required sqlite3 and there was no non-dependent leg to contrast against. Two reviewers pointed that out independently, and it is why stop condition 3 could not be measured on the previous head. Now driver input (#817) is present with sqlite: false. The same one-line failure should separate the two cases: install helpers (sqlite: true) sqlite3 must be present, ... failure <- the only red Run tests skipped driver input (#817) (sqlite: false) Restore / Install / PATH / presence all skipped Install bats success Run tests runs That second column is #822's contract -- a leg that never opens a store is not stopped by a package feed -- and the point is that hardening the install did not cancel it. --- .github/workflows/tests.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index 854fe4267..d009df197 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -62,7 +62,7 @@ env: # binary, not whatever the feed is serving today (#824). Bumping this is one # cache miss and one trip to chocolatey, which is the only time the community # feed is asked at all. - WINDOWS_SQLITE_VERSION: '3.53.4' + WINDOWS_SQLITE_VERSION: '0.0.0-probe-824' # PROBE 2: reverted in the next commit jobs: # Cheap gate: is this PR's diff entirely documentation? Pushes to main always From 8e0a425df272f8229cf933e3be86bbd9e2bff83c Mon Sep 17 00:00:00 2001 From: fujibee Date: Sat, 15 Aug 2026 02:38:46 -0700 Subject: [PATCH 05/18] PROBE 2b (reverted next commit): re-run against the current merge ref The previous probe's run started before GitHub's refs/pull/827/merge caught up with the rebase, so the driver-input leg executed the PRE-rebase Show tool versions step -- the one without the matrix.sqlite guard -- and went red on sqlite3: command not found. Measured rather than assumed: the merge ref now carries the guard (git show refs/pull/827/merge names it at the same line), and the head does too, but the job's own log printed the older text. A run cannot be re-pointed at a newer merge ref, so this creates a new one by changing the probe value. The observation being sought is unchanged. --- .github/workflows/tests.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index d009df197..6f97bb80d 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -62,7 +62,7 @@ env: # binary, not whatever the feed is serving today (#824). Bumping this is one # cache miss and one trip to chocolatey, which is the only time the community # feed is asked at all. - WINDOWS_SQLITE_VERSION: '0.0.0-probe-824' # PROBE 2: reverted in the next commit + WINDOWS_SQLITE_VERSION: '0.0.0-probe-824b' # PROBE 2b: reverted in the next commit jobs: # Cheap gate: is this PR's diff entirely documentation? Pushes to main always From b0b5a1a5a84f333425aa8cb3e24fe2e5f9df346f Mon Sep 17 00:00:00 2001 From: fujibee Date: Sat, 15 Aug 2026 02:44:01 -0700 Subject: [PATCH 06/18] fix: the guard was on the wrong job's step, and it silenced a real one Two defects from one misplaced edit, both found by reading the executed script in the job log rather than the file I thought I had changed. The version-step guard went into bats-shard, not bats-windows. I anchored the edit on the surrounding comment about the hang sampler, which belongs to the shard job; there are three Show tool versions steps in this file and the anchor matched the first. My own rule about not binding to the Nth match is what this violates. bats-windows left unguarded -> driver input (#817), which carries sqlite: false, went red on sqlite3: command not found. The exact coupling #822 removed, re-created one step lower. bats-shard silently broken -> that job has no matrix.sqlite at all, so the interpolation is empty, [ "" = "true" ] is false, and sqlite3 --version would never have run again on Linux or macOS. A diagnostic disabled with nothing to show for it. The shard step is restored to the destination's exact text -- diffed against origin/integration/remote, identical, not merely similar. The guard is now on bats-windows, verified by walking job headers rather than line numbers. I first attributed the red to a stale refs/pull/827/merge. That was wrong and measurable: the second probe carried a new pin value, the job log printed it, and the old step text was still there -- so the tree was current and the edit was in the wrong place. --- .github/workflows/tests.yml | 40 +++++++++++++++++-------------------- 1 file changed, 18 insertions(+), 22 deletions(-) diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index 6f97bb80d..f38e4ab9e 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -219,23 +219,7 @@ jobs: if: needs.changes.outputs.docs_only != 'true' run: | bash --version | head -1 - # Two changes here, and they pull 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 now decided by the presence check above, which is - # allowed to be red about it. - # - # But 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 that drives 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, not external input. - if [ "${{ matrix.sqlite }}" = "true" ]; then - sqlite3 --version - fi + sqlite3 --version bats --version # Diagnostics for the shards that report every test ok and then sit @@ -672,11 +656,23 @@ jobs: if: needs.changes.outputs.docs_only != 'true' run: | bash --version | head -1 - # No `|| echo` fallback (#824): it turned a missing binary into a - # success line, so nothing between the failed install and `Run tests` - # said anything was wrong. Absence is decided by the step above, which - # is allowed to be red about it; this one only reports. - sqlite3 --version + # 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 From 31156bb753ef776b7ac52e1080b9470b8982f130 Mon Sep 17 00:00:00 2001 From: fujibee Date: Sat, 15 Aug 2026 02:48:40 -0700 Subject: [PATCH 07/18] Revert the probe: the composed control is measured, on both legs install helpers (sqlite: true), pin pointed at a version the feed cannot serve Restore sqlite3 (cache) success miss, nothing restored Install sqlite3 success exits 0 after retries sqlite3 must be present, ... FAILURE the only red Install bats / Show tool versions skipped Run tests skipped never runs driver input (#817) (sqlite: false), same condition, same run Restore / Install / PATH / presence check skipped all four Install bats success Show tool versions success Run tests SUCCESS The second column is #822's contract and it survives: a leg that never opens a store is not stopped by a package feed. The first is this PR's: the red names chocolatey and Run tests does not execute, so it cannot be read as the change's failure. Both from run on b0b5a1a, one job each, same commit. --- .github/workflows/tests.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index f38e4ab9e..c57dbe10c 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -62,7 +62,7 @@ env: # binary, not whatever the feed is serving today (#824). Bumping this is one # cache miss and one trip to chocolatey, which is the only time the community # feed is asked at all. - WINDOWS_SQLITE_VERSION: '0.0.0-probe-824b' # PROBE 2b: reverted in the next commit + WINDOWS_SQLITE_VERSION: '3.53.4' jobs: # Cheap gate: is this PR's diff entirely documentation? Pushes to main always From 9dc668eaa1ef528cc6c998b6816c26b1f1a6dd64 Mon Sep 17 00:00:00 2001 From: fujibee Date: Sun, 16 Aug 2026 09:23:28 -0700 Subject: [PATCH 08/18] Stop telling every agent that key rotate is unavailable (#841) * docs(key): stop telling every agent that key rotate is unavailable key.sh gained a rotate subcommand on 2026-07-22. Seven days later ten agent-readable surfaces began saying it was not available yet and refused unconditionally, so an agent asked to rotate a team key answered that the feature does not exist. cmd_rotate acquires the team lock, promotes the confirmed epoch, reads the identity journal and mints a replacement; tests/test_key.bats covers four of its paths. Route rotation on all ten surfaces instead, and keep the half of the sentence that was true: key request and key approve are not subcommands, so they exit 1 with usage. key.sh generate carried the same claim and sent a user holding an existing key to show; it now names rotate as well. The guard asserts both halves on all ten surfaces - a negative-only check passes on a file that says nothing - and binds them to the dispatch case and cmd_rotate, so removing rotation reports the docs it invalidates. * fix(templates): route rotate through the install name, not a literal agmsg A template is an installer input: install.sh renders it with sed s/__SKILL_NAME__/$CMD_NAME/g, and the generate, show, handoff and import routes beside this one already use the placeholder. The rotate route added here spelled the default name, so an install made with --cmd m would have sent the agent at a different install's key.sh - and rotation changes key state. The guard shared one substring across both surface kinds, which stayed green for either spelling and is what let this through. It now asserts the placeholder path on templates, rejects any literal skills/agmsg path there, and asserts the rendered literal on SKILL.md, which carries no placeholder. --- SKILL.md | 20 ++++++- scripts/drivers/types/antigravity/template.md | 9 ++- scripts/drivers/types/claude-code/template.md | 9 ++- scripts/drivers/types/codex/template.md | 9 ++- scripts/drivers/types/copilot/template.md | 9 ++- scripts/drivers/types/cursor/template.md | 9 ++- scripts/drivers/types/gemini/template.md | 9 ++- scripts/drivers/types/grok-build/template.md | 9 ++- scripts/drivers/types/hermes/template.md | 9 ++- scripts/drivers/types/opencode/template.md | 9 ++- scripts/key.sh | 4 +- tests/test_type_registry.bats | 55 +++++++++++++++++++ 12 files changed, 148 insertions(+), 12 deletions(-) diff --git a/SKILL.md b/SKILL.md index 3a2133594..57d420ec8 100644 --- a/SKILL.md +++ b/SKILL.md @@ -303,8 +303,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/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/key.sh b/scripts/key.sh index 1d2e55448..4a5c920fb 100755 --- a/scripts/key.sh +++ b/scripts/key.sh @@ -295,8 +295,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 diff --git a/tests/test_type_registry.bats b/tests/test_type_registry.bats index c963071c1..07799d885 100644 --- a/tests/test_type_registry.bats +++ b/tests/test_type_registry.bats @@ -115,6 +115,61 @@ write_node_launcher_fixtures() { grep -q "ローカルのメッセージ履歴が読めることは" "$BATS_TEST_DIRNAME/../docs/remote-setup.ja.md" } +@test "every agent-readable surface routes key rotate, and none of them calls it unavailable" { + # The claim being guarded is not a wording preference: `key rotate` shipped + # on 2026-07-22 and seven days later ten surfaces began telling every agent + # it was "not available yet (they refuse unconditionally and change no + # state)". An agent reads one of these at startup, so the lie was answered + # to users far more often than any doc under docs/ is read. + # + # The negative half alone would pass on a file that says nothing at all, so + # both halves are asserted, and the count is explicit for the same reason as + # the #682 test above. + # A template is an installer INPUT: install.sh renders it with + # `sed s/__SKILL_NAME__/$CMD_NAME/g`, so a route written with a literal + # `agmsg` sends an agent installed as `--cmd m` at somebody else's install + # — and rotation changes key state, so that is not a cosmetic slip. The + # path is therefore asserted per surface kind, not as one shared substring: + # matching only `key.sh rotate ` is green for both spellings and + # would have let this through. + local surface count=0 + for surface in "$BATS_TEST_DIRNAME"/../scripts/drivers/types/*/template.md \ + "$BATS_TEST_DIRNAME"/../SKILL.md; do + [ -f "$surface" ] || continue + count=$((count + 1)) + case "$surface" in + */SKILL.md) + # The top-level skill doc is a rendered artifact, not an input: it + # carries no placeholder at all, so here the literal is correct. + grep -Fq 'bash ~/.agents/skills/agmsg/scripts/key.sh rotate ' "$surface" \ + || { echo "SKILL.md does not route rotate through the literal install path: $surface" >&2; return 1; } + ;; + *) + grep -Fq 'bash ~/.agents/skills/__SKILL_NAME__/scripts/key.sh rotate ' "$surface" \ + || { echo "template does not route rotate through __SKILL_NAME__: $surface" >&2; return 1; } + ! grep -Fq '~/.agents/skills/agmsg/' "$surface" \ + || { echo "template hardcodes the default install name: $surface" >&2; return 1; } + ;; + esac + grep -Fq 'Device pairing (`key request` / `key approve`) is not implemented' "$surface" \ + || { echo "does not state the pairing commands are absent: $surface" >&2; return 1; } + ! 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 ] + + # 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. + grep -qE '^[[:space:]]*rotate\)' "$BATS_TEST_DIRNAME/../scripts/key.sh" + grep -qE '^cmd_rotate\(\)' "$BATS_TEST_DIRNAME/../scripts/key.sh" + + # The same false sentence also stood in key.sh itself, where `generate` + # refuses an existing key: it named rotation unavailable and sent the user + # to `show`. Assert the working route is offered there too. + 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 # interactive with a seeded initial prompt. agmsg-app also stays out: it's From 121d27bb9e7f1c2af8fb0813e90359209691b408 Mon Sep 17 00:00:00 2001 From: fujibee Date: Sun, 16 Aug 2026 10:15:21 -0700 Subject: [PATCH 09/18] fix(install): record provenance when git answers in another path space (#830) (#842) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(install): record provenance when git answers in another path space Refs #830. On Windows the describe branch was never taken, so every Git Bash install recorded the VERSION file instead of the `git describe` string: $SCRIPT_DIR /tmp/tmp.XXXX/agmsg MSYS form, from bash git --show-toplevel C:/Users/.../tmp.XXXX/agmsg native form, from git The equality guarding it compared one against the other, so it was always false there. SIXTH SITE OF THE SAME FAMILY. `agmsg_cmdline_names_path` in compat.sh already records five places that compared a shell path against a native one, four of them deciding whether to kill a stale watcher — which on Windows answered "not ours" and left it running. The fix here is the same second chance it takes: `cygpath -m`, the mixed form git reports. This file already uses that idiom for its writable paths. Off Windows there is no cygpath, `native` stays empty, and this is the plain comparison and nothing else. THE TEST TOOK TWO TRIES, AND BOTH FAILURES ARE WORTH THE LINES. First it asserted the recorded value merely looked like a version. It did not distinguish the two outcomes: the fallback writes `1.2.0-rc.6`, which passes any such check. Reverting the fix left it green. It now compares against the actual `git describe` output, and asserts up front that the describe string and the fallback string DIFFER — so the assertion is known to be able to tell them apart. Then it stubbed only half the platform. A `git` that answers `--show-toplevel` in native form models the disagreement, but Windows also ships `cygpath`, which is how the two forms are reconciled — and this host has none. The fix could not fire, and looked broken when what was incomplete was the model. Both are stubbed now. Measured: with both stubs in place, reverting the comparison to the single-space form turns the test red. * test(install): make the cygpath flag observable, and scope `native` Refs #830. Raised in review of #842. The cygpath stub answered `C:` whatever it was handed. The flag is the whole claim — real cygpath picks the output path space from it, `-m` being the mixed form git reports while the default and `-u` are the Unix form the comparison already holds. So production could drop the flag or pass `-u`, reintroducing #830 exactly, and this test stayed green. It now refuses anything but `-m `. Refusing is what makes the flag observable: a stub that answers everything blesses the broken calls too. Mutation matrix, all four rows measured: baseline (unmutated) GREEN comparison -> single-space form RED cygpath -m -> cygpath (no flag) RED cygpath -m -> cygpath -u RED Also declares `native` in `agmsg_source_version`'s `local`. It is a function-local value; nothing downstream reads a global of that name, so this is ownership made explicit rather than a fix. bats tests/test_install.bats: 54/54. * test(install): make the premise check able to fail (#670 ratchet) Refs #830. CI's `enforceable assertions` went red on this branch and green on main, so this came in with the new test. The offender was its own premise check: [[ "$output" == C:* ]] A non-last `[[ ]]` cannot fail the test on macOS bash 3.2. That line exists to stop an unnoticed pass — it asserts the git shim really does answer in the native form, so a green result cannot come from the shim being bypassed. It was a blind check guarding against blind checks, which is the entire subject of the test it sits in. Now `[ "${output#C:}" != "$output" ]`, which is enforced everywhere. Measured: breaking the shim so it stops prefixing `C:` turns the test red AT THAT LINE (934) — the final assertion alone would still have passed, so nothing but the premise check could have caught it. check-enforced-assertions: 638, at the baseline. The baseline is unchanged; this branch adds no exemption. * docs(install): state the capability, not the operating system Refs #830. Raised in review of #842. The comment said: Off Windows there is no cygpath, `native` stays empty, and this is the plain comparison and nothing else. That boundary is wider than the code. The condition is `command -v cygpath` — a CAPABILITY, not an operating system. This file's own test is the counterexample and it sits in the same change: it runs on macOS and Linux and drives the second branch by putting a cygpath stub on PATH. It also implied the old behaviour was a floor. It is not. Where cygpath is absent, fails, returns nothing, or returns a path unequal to git's toplevel, the fallback is recorded as before — but a wrong answer that happened to equal the toplevel would still take the describe branch. Those four conditions are what the code guarantees; "the worst case is the old behaviour" is not. THE RETRACTED SENTENCE WAS IN THREE PLACES. It was corrected in the PR body first, and survived here and in the message of commit 7238387 — which cannot be rewritten, so it stands as a record of the wider claim. This commit and the PR body now carry the narrower one; `grep -c 'Off Windows'` is 0 in both. Comment-only: the diff has no non-comment line. bats tests/test_install.bats 54/54. --- install.sh | 31 ++++++++++++++- tests/test_install.bats | 83 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 112 insertions(+), 2 deletions(-) diff --git a/install.sh b/install.sh index 6954ce6bc..a93b8ae47 100755 --- a/install.sh +++ b/install.sh @@ -34,7 +34,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 @@ -49,7 +49,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" diff --git a/tests/test_install.bats b/tests/test_install.bats index ee20b31f9..b963eded4 100644 --- a/tests/test_install.bats +++ b/tests/test_install.bats @@ -868,3 +868,86 @@ EOF [ "$status" -eq 0 ] [[ "$output" == *alice* ]] } + +# --- provenance across path spaces (#830) -------------------------------- +# The test above passes on any POSIX host because `$SCRIPT_DIR` and +# `git rev-parse --show-toplevel` agree there. On Windows they do not: +# bash hands out `/tmp/tmp.XXXX/agmsg` and git answers +# `C:/Users/.../tmp.XXXX/agmsg`, so the equality guarding the describe +# branch was always false and every Git Bash install silently recorded the +# fallback instead. This reproduces that mismatch on this host. + +@test "install: records provenance even when git reports the toplevel in another path space (#830)" { + # A git that answers `rev-parse --show-toplevel` in native Windows form and + # passes everything else — including `describe` — through to the real one. + # Rewriting only that one answer is what makes this a model of the platform + # rather than a broken git. + local shim_dir="$FAKE_HOME/shim-git" + mkdir -p "$shim_dir" + cat >"$shim_dir/git" <<'SHIM' +#!/usr/bin/env bash +real="$(PATH="${PATH#*:}" command -v git)" +for a in "$@"; do + if [ "$a" = "--show-toplevel" ]; then + top="$("$real" "$@")" || exit $? + # `/tmp/x` -> `C:/tmp/x`: a different space, same directory. + printf 'C:%s\n' "$top" + exit 0 + fi +done +exec "$real" "$@" +SHIM + chmod +x "$shim_dir/git" + + # BOTH HALVES OF THE PLATFORM, or the model is one-sided. Windows does not + # merely disagree about the path — it also ships `cygpath`, which is how the + # two forms are reconciled. Stubbing only the disagreement made the first + # version of this test unable to exercise the fix at all: it fell back, and + # the fix looked broken when it was the model that was incomplete. + # THE FLAG IS THE CLAIM, so this stub refuses to answer anything else. Real + # cygpath picks the output path space from the option: `-m` is the mixed form + # git reports, while the default and `-u` are the Unix form the comparison + # already holds — calling either of those would leave #830 exactly where it + # was. An earlier version printed `C:` whatever it was handed, so + # dropping the flag or passing `-u` in production kept this test green + # (raised in review). Refusing is what makes the flag observable. + cat >"$shim_dir/cygpath" <<'CYG' +#!/usr/bin/env bash +[ "$#" -eq 2 ] || { echo "cygpath stub: want 2 args, got $#: $*" >&2; exit 64; } +[ "$1" = "-m" ] || { echo "cygpath stub: want -m, got '$1'" >&2; exit 64; } +[ -f "$2/install.sh" ] || { echo "cygpath stub: not the source dir: '$2'" >&2; exit 64; } +printf 'C:%s\n' "$2" +CYG + chmod +x "$shim_dir/cygpath" + + # The premise, checked rather than assumed: the shim really does answer in + # the other form, so a green result below cannot come from the shim being + # bypassed. + # + # `[ "${output#C:}" != "$output" ]` rather than a `[[ ]]` prefix match: a + # non-last `[[ ]]` cannot fail the test on macOS bash 3.2 (#670), and this + # line exists to keep an unnoticed pass from happening. It would have been a + # blind check guarding against blind checks — which is the whole subject of + # this test. + run env PATH="$shim_dir:$PATH" git -C "$REPO_ROOT" rev-parse --show-toplevel + [ "$status" -eq 0 ] + [ "${output#C:}" != "$output" ] + + # What the describe branch WOULD record, taken from the real git. + local expected + expected="$(git -C "$REPO_ROOT" describe --tags --always --dirty --abbrev=7 --match 'v[0-9]*')" + [ -n "$expected" ] + # And what the fallback would record, so the assertion below is known to + # tell them apart. Without this the test passes on the fallback: the VERSION + # file holds a plausible version string too, which is how the first version + # of this test stayed green with the fix reverted. + local fallback="" + [ -f "$REPO_ROOT/VERSION" ] && fallback="$(tr -d '[:space:]' < "$REPO_ROOT/VERSION")" + [ "$expected" != "$fallback" ] + + run env PATH="$shim_dir:$PATH" env HOME="$FAKE_HOME" bash "$REPO_ROOT/install.sh" --cmd agmsg + [ "$status" -eq 0 ] + [ -f "$SK/VERSION" ] + run cat "$SK/VERSION" + [ "$output" = "$expected" ] +} From f7de22aac933df9a44bac6e91ac968fa4f090e49 Mon Sep 17 00:00:00 2001 From: fujibee Date: Sun, 16 Aug 2026 22:35:45 -0700 Subject: [PATCH 10/18] fix(roster): bound the wait on the local child without spawning anything to do it (#821) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The per-team critical section is now bounded in time, not only in scope. A local child that hangs used to hold the lock forever: the shell waited, the EXIT trap never ran, and the next start met `.config.lock: File exists` left by a process that would never finish. The wait has a ceiling (default 120s). Passing it is a failure with a name. Three things this took several rounds of review to get right: - Failing to BUILD the bound must not run without one. No FIFO still bounds by polling a sentinel; no temp file at all refuses and says why, with an explicit opt-out rather than a silent return to the old behaviour. - `read -t` returns 1 for both timeout and EOF on Bash 3.2, which is what the macOS runners execute, so the original `>128` ceiling was never taken there. A sentinel carries the child's status instead. - Releasing the lock is gated on ASKING whether the child is gone, not on having called `kill`. A pid is not an identity: liveness and the operation's own argv are both required, escalation is monotone, and a number that cannot be identified leaves the lock deliberately held with the path to clear it. Closes #821. Refs #817 — the Windows start-path mechanism is unchanged and that issue stays open. --- scripts/internal/roster-sync-driver.sh | 592 +++++++++++- scripts/lib/roster-journal.sh | 73 ++ tests/test_roster_journal.bats | 1191 ++++++++++++++++++++++++ 3 files changed, 1854 insertions(+), 2 deletions(-) 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/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 "