From 9ec81d08c7022f71a653ae9d76a39e187d1e9cf8 Mon Sep 17 00:00:00 2001 From: fujibee Date: Fri, 14 Aug 2026 03:51:13 -0700 Subject: [PATCH 1/2] feat(sync): keep the engine up when the server refuses, and let the reason be read MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A remote may answer a write with a status meaning the caller must do something, rather than try again later. The engine treated that as a transport failure: not retryable, so it left the loop, and the process exited. status then said 'engine stopped — run: remote.sh sync start', which invites the one action that cannot work — starting it again produces the same refusal and the same exit, for as long as the server's answer stands (#773). THE REASON WAS NEVER MISSING. event() writes it to the run log with a timestamp, at a known path. What was missing is a place to READ it: status opens a pidfile and the cycle stamp, and nothing an agent consults mentions the log. So this adds one fact beside the cycle stamp, not a new mechanism. BY CLASS, NOT BY NUMBER. A refusal is a 4xx the retry policy does not cover. The one such status in use today is deliberately not named anywhere in the engine — not in the code and not in a comment, which a test enforces. Naming it invites the next reader to special-case it and the one after that to add a sentence about what it means. 5xx stays transport; the retryable 4xx stay retryable, because isRetryable is asked first. THE ENGINE STAYS UP. A refusal is not retryable — asking again does not change a decision — and not a transport failure either. Exiting turns a recoverable condition into a dead process whose status recommends a no-op. And the answer can change out of band: someone pays, a quota resets, an operator fixes a setting. Staying up means the next cycle recovers with nobody typing anything. It backs off to the longest interval and does not advance the failure count, because a refusal is not evidence that the transport is degrading. WHAT IS STORED AND WHAT IS NOT. Stored: status, code, time, and the host from the endpoint the config already holds — verbatim, as the server said them. Not stored: any sentence about what the refusal means or what to do. This engine talks to a remote that may be self-hosted, someone else's, or a service, and a sentence it invents is wrong for some server. Interpretation belongs to whoever operates that server; the host is there so a reader knows who that is. Cleared by the next successful cycle, beside where the success is recorded. A record that outlives its truth is worse than none: status would keep reporting a decision the server has since reversed. THE AGENT'S SURFACE. remote.sh status --json is what /agmsg remote status runs, and the only thing an agent consults about the engine. The whole record is passed through, so a field added by the engine needs no change there; an unreadable record reports as absent rather than guessed at. Tests: 4 in the engine suite (classification by class, the loop not exiting, a success clearing the record, and a non-refusal still ending the loop) and 6 in bats (the wording repeated not interpreted, an unenumerated status surviving, the JSON carrying it verbatim, null rather than a missing key, an unreadable record read as absent, and that the engine names no particular remote — with a positive control on that search). Not here: the auto-start reading the record before starting a team. That lives in scripts/lib/sync-autostart.sh, which does not exist on this branch — it arrives with #774, so those lines land there and are reviewed there. --- scripts/internal/remote-sync.mjs | 134 +++++++++++++++++++++++++++++- scripts/remote.sh | 62 +++++++++++++- tests/remote_sync_engine.test.mjs | 103 +++++++++++++++++++++++ tests/test_remote_refusal.bats | 115 +++++++++++++++++++++++++ 4 files changed, 411 insertions(+), 3 deletions(-) create mode 100644 tests/test_remote_refusal.bats diff --git a/scripts/internal/remote-sync.mjs b/scripts/internal/remote-sync.mjs index 4bb4d960..712a0187 100755 --- a/scripts/internal/remote-sync.mjs +++ b/scripts/internal/remote-sync.mjs @@ -2,7 +2,7 @@ import { createHash } from "node:crypto"; import { constants } from "node:fs"; import { spawn } from "node:child_process"; -import { appendFile, lstat, mkdir, open, readFile, rename, rmdir, stat, writeFile } from "node:fs/promises"; +import { appendFile, lstat, mkdir, open, readFile, rename, rm, rmdir, stat, writeFile } from "node:fs/promises"; import { basename, dirname, join, resolve, sep } from "node:path"; import process from "node:process"; import { realpathSync } from "node:fs"; @@ -142,6 +142,98 @@ async function recordCycleSuccess(team, at, writeFileCall = writeFile) { } catch { /* best-effort: never fail a working cycle over its own bookkeeping */ } } +// A refusal the caller can act on, recorded where something can read it (#773). +// +// A remote may answer a write with a status meaning "the caller must do +// something" rather than "try again later". The engine used to treat that as a +// transport failure: not retryable, so it left the loop, and the process +// exited. `status` then said "engine stopped -- run: remote.sh sync start", +// which invites the one action that cannot work; starting it again produces +// the same refusal and the same exit, for as long as the server's answer +// stands. +// +// THE REASON WAS NEVER MISSING. `event()` writes `fatal` to stdout and +// `sync start` captures it into `run/remote-sync..log`, with a +// timestamp, at a known path. What was missing is a place to READ it from: +// `status` opens a pidfile and the cycle stamp, and nothing an agent consults +// mentions the log. So this is one more fact beside the cycle stamp, not a new +// mechanism -- the same move #760 made. +// +// WHAT IS STORED, AND WHAT IS DELIBERATELY NOT. +// +// Stored: the status, the code, the time, and the host from the endpoint the +// config already holds. Verbatim, as the server said them. +// +// Not stored: any sentence about what the refusal MEANS or what to do about +// it. This engine talks to *a* remote -- self-hosted, someone else's, or a +// service -- and it cannot know why a particular one refused. A sentence it +// invents is wrong for some server. Interpretation belongs to whoever operates +// that server, and the host is there so a reader knows who that is. +function refusalPath(team) { + const connectionRoot = process.env.AGMSG_SYNC_CONNECTION_DIR ?? process.env.SKILL_DIR; + if (!connectionRoot) throw new Error("sync connection root is unavailable"); + return join(connectionRoot, "run", `remote-sync.${team}.refusal.json`); +} + +/** + * A refusal is a 4xx the retry policy does not cover. + * + * BY CLASS, NOT BY NUMBER. There is one such status in use today, and it is + * deliberately not named anywhere in this file — not in the code and not in + * this comment, which a check enforces. A server may refuse for reasons this + * protocol never enumerates, and every one of them is "the server decided, and + * said so" rather than "ask again later"; naming one would invite the next + * reader to special-case it, and the one after that to add a sentence about + * what it means. + * + * Everything in the 4xx range that the retry policy does not cover lands here, + * for that same reason. 5xx stays a transport failure and the retryable + * statuses stay retryable, because `isRetryable` is asked first. + * + * 5xx stays a transport failure, and the retryable 4xx (408, 429) stay + * retryable, because `isRetryable` is asked first. + */ +/** The host of an endpoint, or null when it cannot be read as a URL. */ +function hostOf(endpoint) { + try { + return new URL(endpoint).host; + } catch { + return null; + } +} + +export function isRefusal(error) { + const status = error?.status; + if (typeof status !== "number") return false; + if (isRetryable(error)) return false; + return status >= 400 && status < 500; +} + +/** + * Write the refusal down. Best-effort, like the cycle stamp and for the same + * reason: bookkeeping must never be the thing that takes syncing down. + */ +async function recordRefusal(team, fact, writeFileCall = writeFile) { + try { + await writeFileCall(refusalPath(team), `${JSON.stringify({ + type: "sync_refusal", ...fact, + })}\n`); + } catch { /* best-effort */ } +} + +/** + * Forget it, because a cycle has since succeeded. + * + * A refusal that outlives its truth is worse than no record: `status` would + * keep reporting a server decision that has been reversed, and the operator + * would keep acting on it. Cleared in the same place the success is recorded. + */ +async function clearRefusal(team, rmCall = rm) { + try { + await rmCall(refusalPath(team), { force: true }); + } catch { /* best-effort */ } +} + // What actually went wrong, when the thing that threw is a wrapper. // // `fetch` rejects with a bare `TypeError: fetch failed` whose own `code` is @@ -2722,6 +2814,9 @@ export async function runLoop(config, options, dependencies = {}) { const eventCall = dependencies.eventCall ?? event; const isRetryableCall = dependencies.isRetryableCall ?? isRetryable; const recordCycleCall = dependencies.recordCycleCall ?? recordCycleSuccess; + const clearRefusalCall = dependencies.clearRefusalCall ?? clearRefusal; + const recordRefusalCall = dependencies.recordRefusalCall ?? recordRefusal; + const isRefusalCall = dependencies.isRefusalCall ?? isRefusal; const nowCall = dependencies.nowCall ?? (() => new Date().toISOString()); // An explicit --limit is a ceiling for BOTH push and pull (request size / @@ -2751,6 +2846,9 @@ export async function runLoop(config, options, dependencies = {}) { // success. Same shape as the `cycle.error` logging below. try { await recordCycleCall(config.local_team, nowCall()); + // A success outdates any refusal on record. Cleared HERE, beside the + // stamp, so the two facts can never disagree about the same cycle. + await clearRefusalCall(config.local_team); } catch { /* bookkeeping is best-effort; the cycle already succeeded */ } catchUp = result?.pushSaturated === true; // catch-up removes the wait ONLY between successful, progress-making @@ -2763,6 +2861,40 @@ export async function runLoop(config, options, dependencies = {}) { try { await eventCall("cycle.error", { message: error.message, ...causeOf(error) }); } catch { /* logging is best-effort */ } + + // A REFUSAL DOES NOT LEAVE THE LOOP (#773). + // + // It is not retryable — asking again does not change a decision — and it + // is not a transport failure either: the server has answered, and said + // what it decided. Exiting on it turns a recoverable condition into a + // dead process whose `status` line recommends starting it again, which + // reproduces the refusal and the exit. + // + // And the answer CAN change, out of band: someone pays, a quota resets, + // an operator fixes a setting. Staying up means the next cycle recovers + // with nobody typing anything. So this backs off to the longest interval + // and keeps asking quietly, and the failure count is not advanced — + // a refusal is not evidence that the transport is degrading. + if (isRefusalCall(error)) { + try { + await recordRefusalCall(config.local_team, { + status: error?.status ?? null, + code: error?.code ?? null, + at: nowCall(), + // From the config the engine already holds. Not an interpretation: + // it says WHERE the operator of that server would be reached. + endpoint_host: hostOf(config.endpoint), + }); + } catch { /* best-effort */ } + try { + await eventCall("cycle.refused", { + status: error?.status ?? null, code: error?.code ?? null, + }); + } catch { /* logging is best-effort */ } + await sleepCall(MAX_BACKOFF_MS); + continue; + } + if (!isRetryableCall(error)) throw error; consecutiveFailures += 1; const backoffMs = Math.min(MAX_BACKOFF_MS, BASE_BACKOFF_MS * 2 ** (consecutiveFailures - 1)); diff --git a/scripts/remote.sh b/scripts/remote.sh index c5a0f2aa..65cc0ed4 100644 --- a/scripts/remote.sh +++ b/scripts/remote.sh @@ -1358,6 +1358,15 @@ _remote_sync_engine_pidfile() { printf '%s' "$CONNECTION_ROOT/run/remote-sync.$1 # Written by the engine after a cycle completes (#756). Derived here the same way # the pidfile is, because it has the same lifetime and the same owner. _remote_sync_engine_cycle_stamp() { printf '%s' "$CONNECTION_ROOT/run/remote-sync.$1.cycles.json"; } +# Written by the engine when the server REFUSED, and removed by the engine on +# the next successful cycle (#773). Same directory, same derivation, same +# lifetime as the two above. +# +# The reason for its existence is that the reason was already on disk and +# unread: `event()` writes a `fatal` line into the run log with a timestamp, +# and nothing `status` opens has ever mentioned it. This is a place to read +# from, not a new place to write. +_remote_sync_engine_refusal() { printf '%s' "$CONNECTION_ROOT/run/remote-sync.$1.refusal.json"; } # _remote_holds_current_key -> 0 when this machine holds the identity # for the team's CURRENT epoch, 1 otherwise. @@ -2133,6 +2142,32 @@ _remote_status_one() { echo " cycles: last successful sync $last_cycle" fi fi + # WHY IT IS NOT SYNCING, when the server has said so (#773). + # + # Printed for a running engine AND a stopped one: a refusal recorded by an + # engine that has since been stopped is still the last thing the server said, + # and the operator asking "why" is owed it either way. + # + # REPEATED, NOT INTERPRETED. The status and the code are the server's words. + # This client talks to *a* remote — self-hosted, someone else's, or a service + # — and cannot know what a particular one means by a particular code. A + # sentence invented here is wrong for some server. What it may add is where + # the operator of that server would be reached, which is the host the + # binding already names. + local refusal_file refusal_status refusal_code refusal_at refusal_host + refusal_file="$(_remote_sync_engine_refusal "$team")" + if [ -f "$refusal_file" ]; then + refusal_status="$(_remote_read_config_field "$refusal_file" '$.status')" + refusal_code="$(_remote_read_config_field "$refusal_file" '$.code')" + refusal_at="$(_remote_read_config_field "$refusal_file" '$.at')" + refusal_host="$(_remote_read_config_field "$refusal_file" '$.endpoint_host')" + if [ -n "$refusal_status" ] && [ "$refusal_status" != "null" ]; then + echo " refused: the server answered $refusal_status${refusal_code:+ $refusal_code} at $refusal_at" + if [ -n "$refusal_host" ] && [ "$refusal_host" != "null" ]; then + echo " that server is $refusal_host — what the answer means is theirs to say" + fi + fi + fi if [ "$binding_cipher" = "age-v1" ]; then if [ -n "$key_id" ] && [ "$key_id" != "null" ]; then echo " encryption: age-v1, key present" @@ -2202,8 +2237,21 @@ _remote_status_json_one() { _remote_config_shape_ok "$raw" || return 2 IFS=$'\t' read -r engine_state engine_pid < <(_remote_sync_engine_status "$team") - printf '%s' "$raw" | python3 -c ' -import json, sys + # THE SURFACE AN AGENT READS (#773). + # + # Someone asks their agent "why isn't this syncing?" and the agent has to be + # able to answer. `/agmsg remote status` runs this with `--json`, and it is + # the only thing an agent consults about the engine — so the refusal has to + # be here, not only in a line a human reads. + # + # Passed as a whole file rather than as parsed fields: whatever the engine + # recorded is what the reader gets, and adding a field there does not need a + # change here. + local refusal_raw="" refusal_file + refusal_file="$(_remote_sync_engine_refusal "$team")" + [ -f "$refusal_file" ] && refusal_raw="$(cat "$refusal_file" 2>/dev/null || true)" + printf '%s' "$raw" | REFUSAL_JSON="$refusal_raw" python3 -c ' +import json, os, sys team, engine_state, engine_pid_text = sys.argv[1:4] try: cfg = json.loads(sys.stdin.read()) @@ -2219,6 +2267,12 @@ engine_pid = int(engine_pid_text) if engine_pid_text else None if state == "disconnected": engine_state = "stopped" engine_pid = None +try: + refusal = json.loads(os.environ.get("REFUSAL_JSON") or "null") +except Exception: + refusal = None +if not isinstance(refusal, dict): + refusal = None print(json.dumps({ "local_team": team, "endpoint": binding.get("endpoint"), @@ -2228,6 +2282,10 @@ print(json.dumps({ "state": state, "engine_state": engine_state, "engine_pid": engine_pid, + # null when the server has refused nothing, or when what was recorded + # cannot be read. An unreadable record is reported as absent rather than + # guessed at; the human line above is derived from the same file. + "refusal": refusal, }, sort_keys=True)) ' "$team" "$engine_state" "$engine_pid" } diff --git a/tests/remote_sync_engine.test.mjs b/tests/remote_sync_engine.test.mjs index 3d3c1b5f..d57c6b2b 100644 --- a/tests/remote_sync_engine.test.mjs +++ b/tests/remote_sync_engine.test.mjs @@ -18,6 +18,7 @@ import { exportAgeSnapshot, isRetryable, initialAgeSnapshot, + isRefusal, runLoop, loadConfig, nextLocalAgeSnapshot, @@ -3388,3 +3389,105 @@ test("an error code is read from the protocol shape, and from the edge's for now assert.equal(errorCode({}), "unknown-error"); assert.equal(errorCode(undefined), "unknown-error"); }); + +// ── a refusal the caller can act on (#773) ────────────────────────────────── +// +// The engine used to leave the loop on one: not retryable, so it threw, main() +// rejected, and the process exited. `status` then said "engine stopped — run: +// remote.sh sync start", which invites the one action that cannot work. + +test("isRefusal: a 4xx the retry policy does not cover, by class and not by number", () => { + // The status in use today is deliberately not named in the engine, so it is + // not named here either — what is asserted is the CLASS. A server may refuse + // for reasons this protocol never enumerates. + for (const status of [400, 401, 402, 403, 409, 422, 451]) { + assert.equal(isRefusal({ status }), true, `${status} should be a refusal`); + } + // Retryable 4xx stay retryable: isRetryable is asked first. + for (const status of [408, 429]) { + assert.equal(isRefusal({ status }), false, `${status} is retryable, not refused`); + } + // 5xx is a transport failure, not a decision. + for (const status of [500, 502, 503, 504]) { + assert.equal(isRefusal({ status }), false, `${status} is transport, not refused`); + } + // An error carrying no status at all — a socket failure — is neither. + assert.equal(isRefusal(new Error("fetch failed")), false); + assert.equal(isRefusal({ retryable: true, status: 402 }), false); +}); + +test("runLoop: a refusal is recorded and does NOT leave the loop", async () => { + const recorded = []; + const sleeps = []; + let i = 0; + // An endpoint the host can actually be read from: the recorded host is the + // one fact here that says WHERE the operator of that server would be + // reached, and a fixture without one would let the assertion pass on null. + const refusedConfig = { ...config, endpoint: "https://sync.example.test" }; + await assert.rejects(() => runLoop(refusedConfig, {}, { + cycleCall: async () => { + i += 1; + if (i <= 2) { const refused = new Error("HTTP 402 payment_required"); refused.status = 402; refused.code = "payment_required"; throw refused; } + // Only a DIFFERENT, non-retryable error ends the loop — which is how + // this test can end at all. If a refusal exited, i would never reach 3. + const stop = new Error("stop"); stop.retryable = false; throw stop; + }, + isRetryableCall: (error) => error.retryable === true, + isRefusalCall: (error) => typeof error.status === "number" && error.status >= 400 && error.status < 500, + recordRefusalCall: async (team, fact) => { recorded.push({ team, ...fact }); }, + clearRefusalCall: async () => {}, + nowCall: () => "2026-08-14T00:00:00Z", + sleepCall: async (ms) => { sleeps.push(ms); }, + eventCall: async () => {}, + }), /stop/); + + // It came back for a second cycle after the first refusal, and a third. + assert.equal(i, 3); + // Both refusals were written down, verbatim, with the host from the config. + assert.equal(recorded.length, 2); + assert.equal(recorded[0].status, 402); + assert.equal(recorded[0].code, "payment_required"); + assert.equal(recorded[0].at, "2026-08-14T00:00:00Z"); + assert.equal(recorded[0].endpoint_host, "sync.example.test"); + // Backed off to the longest interval rather than hammering, and the failure + // count was not advanced — a refusal is not evidence the transport is + // degrading, so it must not shorten anything else's backoff. + assert.deepEqual(sleeps, [60000, 60000]); +}); + +test("runLoop: a successful cycle clears a refusal that is no longer true", async () => { + // A record that outlives its truth is worse than no record: `status` would + // keep reporting a decision the server has since reversed. + const cleared = []; + let i = 0; + await assert.rejects(() => runLoop(config, {}, { + cycleCall: async () => { + i += 1; + if (i === 1) { const refused = new Error("refused"); refused.status = 402; throw refused; } + if (i === 2) return { pushSaturated: false }; + const stop = new Error("stop"); stop.retryable = false; throw stop; + }, + isRetryableCall: (error) => error.retryable === true, + isRefusalCall: (error) => typeof error.status === "number" && error.status >= 400 && error.status < 500, + recordRefusalCall: async () => {}, + clearRefusalCall: async (team) => { cleared.push(team); }, + recordCycleCall: async () => {}, + nowCall: () => "2026-08-14T00:00:00Z", + sleepCall: async () => {}, + eventCall: async () => {}, + }), /stop/); + assert.deepEqual(cleared, [config.local_team]); +}); + +test("runLoop: a non-retryable error that is NOT a refusal still ends the loop", async () => { + // The negative control. Staying up for everything would turn a malformed + // config into an engine that spins forever saying nothing useful — exiting + // is right for that, and the refusal case is the exception, not the rule. + await assert.rejects(() => runLoop(config, {}, { + cycleCall: async () => { const bad = new Error("config is unreadable"); throw bad; }, + isRetryableCall: () => false, + isRefusalCall: () => false, + sleepCall: async () => {}, + eventCall: async () => {}, + }), /config is unreadable/); +}); diff --git a/tests/test_remote_refusal.bats b/tests/test_remote_refusal.bats new file mode 100644 index 00000000..f8237fc0 --- /dev/null +++ b/tests/test_remote_refusal.bats @@ -0,0 +1,115 @@ +#!/usr/bin/env bats + +load test_helper + +# A refusal the operator could act on, recorded and readable (#773). +# +# The engine used to treat one as a transport failure: not retryable, so it +# left the loop, and the process exited. `status` then said "engine stopped — +# run: remote.sh sync start", which invites the one action that cannot work. +# +# The reason was never missing — `event()` writes it to the run log — it was +# unread. So what is tested here is the reading: a place `status` opens, and +# the JSON an agent consults, carrying what the server said and nothing this +# client invented. + +setup() { + setup_test_env + bash "$SCRIPTS/join.sh" testteam alice claude-code /tmp/project-a + + local cfg="$TEST_SKILL_DIR/teams/testteam/config.json" escaped updated + escaped="$(sed "s/'/''/g" "$cfg")" + updated="$(sqlite_mem " + SELECT json_set('$escaped', '\$.remote_binding', json_object( + 'endpoint', 'https://sync.example.test', + 'server_instance_id', '018f0000-0000-7000-8000-000000000001', + 'remote_team_id', '018f0000-0000-7000-8000-000000000002', + 'protocol_version', 1, + 'capabilities', json_object('write_allowed_ciphers', json_array('none')), + 'connected_at', '2026-07-30T00:00:00Z', + 'disconnected_at', null + ));")" + printf '%s\n' "$updated" > "$cfg" + mkdir -p "$TEST_SKILL_DIR/run" +} + +teardown() { teardown_test_env; } + +# What the engine writes when the server refuses. Written here rather than by +# running the engine: this file is about what READS it. +write_refusal() { + printf '%s\n' "{\"type\":\"sync_refusal\",\"status\":${1:-402},\"code\":\"${2:-payment_required}\",\"at\":\"2026-08-14T00:00:00Z\",\"endpoint_host\":\"sync.example.test\"}" \ + > "$TEST_SKILL_DIR/run/remote-sync.testteam.refusal.json" +} + +@test "status says what the server answered, and does not say what it meant" { + write_refusal 402 payment_required + run bash "$SCRIPTS/remote.sh" status testteam + [ "$status" -eq 0 ] + printf '%s' "$output" | grep -q 'refused: the server answered 402 payment_required' + # WHERE the operator of that server is, which the binding already knows. + printf '%s' "$output" | grep -q 'sync.example.test' + # And nothing this client invented about what it MEANS. A sentence here + # would be wrong for some server, and every one of these is a sentence only + # the operator of that server may write. + refute grep -qi 'subscri' <<<"$output" + refute grep -qi 'upgrade' <<<"$output" + refute grep -qi 'billing' <<<"$output" + refute grep -qi 'plan' <<<"$output" +} + +@test "status repeats a status this protocol never enumerated" { + # BY CLASS, NOT BY NUMBER. A self-hosted server may refuse for its own + # reasons with a code nothing here has heard of, and the answer must survive + # the trip unchanged. + write_refusal 451 tenant_suspended_by_operator + run bash "$SCRIPTS/remote.sh" status testteam + printf '%s' "$output" | grep -q '451 tenant_suspended_by_operator' +} + +@test "the agent's surface carries it, verbatim" { + # `/agmsg remote status` runs this. It is the only thing an agent consults + # about the engine, so a refusal that reached only the human line would + # leave "why isn't this syncing?" unanswerable. + write_refusal 402 payment_required + run bash "$SCRIPTS/remote.sh" status testteam --json + [ "$status" -eq 0 ] + local got + got="$(printf '%s' "$output" | python3 -c 'import json,sys; print(json.load(sys.stdin)["refusal"]["status"])')" + [ "$got" = "402" ] + got="$(printf '%s' "$output" | python3 -c 'import json,sys; print(json.load(sys.stdin)["refusal"]["code"])')" + [ "$got" = "payment_required" ] + got="$(printf '%s' "$output" | python3 -c 'import json,sys; print(json.load(sys.stdin)["refusal"]["endpoint_host"])')" + [ "$got" = "sync.example.test" ] +} + +@test "no refusal is null, not a missing key" { + # A consumer that has to tell "absent" from "unreadable" needs the key to be + # there either way. + run bash "$SCRIPTS/remote.sh" status testteam --json + [ "$status" -eq 0 ] + local got + got="$(printf '%s' "$output" | python3 -c 'import json,sys; d=json.load(sys.stdin); print("KEY" if "refusal" in d else "MISSING", d["refusal"])')" + [ "$got" = "KEY None" ] +} + +@test "an unreadable record reads as absent, not as a guess" { + printf '%s\n' 'not json at all' > "$TEST_SKILL_DIR/run/remote-sync.testteam.refusal.json" + run bash "$SCRIPTS/remote.sh" status testteam --json + [ "$status" -eq 0 ] + local got + got="$(printf '%s' "$output" | python3 -c 'import json,sys; print(json.load(sys.stdin)["refusal"])')" + [ "$got" = "None" ] +} + +@test "the engine carries no knowledge of any particular remote" { + # The requirement that is easiest to lose later, so it is a check rather + # than a note. `402` is the case that exists today and the engine classifies + # by CLASS — a 4xx the retry policy does not cover — so the number has no + # business being in there. + refute grep -n '402' "$SCRIPTS/internal/remote-sync.mjs" + refute grep -ni 'payment_required' "$SCRIPTS/internal/remote-sync.mjs" + # A positive control on the search itself: it has to be able to find + # something in that file, or these three prove nothing. + grep -q 'isRefusal' "$SCRIPTS/internal/remote-sync.mjs" +} From 288545154f84e334460eec9d1e6e934170afceeb Mon Sep 17 00:00:00 2001 From: fujibee Date: Fri, 14 Aug 2026 04:09:01 -0700 Subject: [PATCH 2/2] fix(sync): do not report a refusal older than the last successful cycle MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The clear was the only thing standing between a reversed decision and the operator, and the clear is allowed to fail. runLoop removes the record after a successful cycle, best-effort, and swallows the failure — an unwritable run directory, a permission change, a crash between the two writes. remote.sh then read the file and reported whatever was in it. So a failed removal meant status and the JSON kept reporting a decision the server had since reversed, for ever. My own body and comment claimed the opposite: that a record outliving its truth is worse than none, and that the two facts are written in one place and so can never disagree. They are written in one place and they still can, because one of the two writes may fail. The standard was right and the implementation did not meet it. THE FIX IS ON THE READING SIDE, and that is the stronger of the two. Making the delete certain would mean retries and escalation in a path whose whole rule is that bookkeeping must never cost a cycle that worked. Instead the reader compares the record to the last successful cycle and reports nothing older. Deleting can fail. Comparing cannot. Both readers go through one helper, so the human line and the JSON cannot disagree about whether a refusal still stands. With no cycle stamp there is nothing to compare against and the refusal IS reported: an engine that has refused and never succeeded is the case the record exists for. The comparison is lexicographic on two timestamps written by the same engine in the same format. That is exact here and is not a general date comparison; the comment says so. Also: reading the record for the comparison is quiet. Saying an unreadable record is unreadable put a parse error into the stream a caller reads the JSON from, which turned the 'unreadable reads as absent' case red. Three tests: a refusal older than the last cycle is not reported THOUGH THE FILE REMAINS (the failed-delete case, made deliberate); a newer one still is (without which the check is satisfied by never reporting anything); and with no stamp at all it is reported. Mutation: remove the comparison -> that one case red, the other eight green. Same shape as #778 in registry-lock.sh, on the same night: cleanup done best-effort, its failure swallowed, and what remains read as the current truth. Passed to that seat. --- scripts/internal/remote-sync.mjs | 19 +++++++++-- scripts/remote.sh | 57 ++++++++++++++++++++++++++++---- tests/test_remote_refusal.bats | 55 ++++++++++++++++++++++++++++++ 3 files changed, 123 insertions(+), 8 deletions(-) diff --git a/scripts/internal/remote-sync.mjs b/scripts/internal/remote-sync.mjs index 712a0187..7ed35e27 100755 --- a/scripts/internal/remote-sync.mjs +++ b/scripts/internal/remote-sync.mjs @@ -226,7 +226,20 @@ async function recordRefusal(team, fact, writeFileCall = writeFile) { * * A refusal that outlives its truth is worse than no record: `status` would * keep reporting a server decision that has been reversed, and the operator - * would keep acting on it. Cleared in the same place the success is recorded. + * would keep acting on it. + * + * BEST-EFFORT, AND NOT WHAT MAKES THAT TRUE. This removal can fail — an + * unwritable run directory, a permission change, a crash between the two + * writes — and it is deliberately not retried or escalated, for the same + * reason `recordCycleSuccess` is not: bookkeeping must never take down a cycle + * that worked. + * + * What makes the guarantee hold is on the READING side: `remote.sh` compares + * the record to the last successful cycle and reports nothing older. Deleting + * can fail; comparing cannot. An earlier version of this comment said the two + * facts were written in the same place and so could never disagree — they are, + * and they still could, because one of the two writes is allowed to fail + * (raised in review). */ async function clearRefusal(team, rmCall = rm) { try { @@ -2847,7 +2860,9 @@ export async function runLoop(config, options, dependencies = {}) { try { await recordCycleCall(config.local_team, nowCall()); // A success outdates any refusal on record. Cleared HERE, beside the - // stamp, so the two facts can never disagree about the same cycle. + // stamp, so a reader that never looks at the timestamps still sees the + // right thing most of the time — but the guarantee is the reader's + // comparison, not this line, because this line may fail. await clearRefusalCall(config.local_team); } catch { /* bookkeeping is best-effort; the cycle already succeeded */ } catchUp = result?.pushSaturated === true; diff --git a/scripts/remote.sh b/scripts/remote.sh index 65cc0ed4..8e92d891 100644 --- a/scripts/remote.sh +++ b/scripts/remote.sh @@ -1368,6 +1368,48 @@ _remote_sync_engine_cycle_stamp() { printf '%s' "$CONNECTION_ROOT/run/remote-syn # from, not a new place to write. _remote_sync_engine_refusal() { printf '%s' "$CONNECTION_ROOT/run/remote-sync.$1.refusal.json"; } +# The refusal record, or nothing when a cycle has SINCE succeeded. +# +# THE READER DOES NOT TRUST THE DELETE. The engine removes this file after a +# successful cycle, and that removal is best-effort — an unwritable run +# directory, a permission change, a crash between the two writes. If the +# removal is the only thing standing between a reversed decision and the +# operator, then a failed removal reports a refusal that is no longer true, +# for ever, on every `status` (raised in review). +# +# So the file is not the answer; the file COMPARED TO THE LAST SUCCESSFUL +# CYCLE is. Deleting can fail. Comparing cannot. That makes this the stronger +# of the two guarantees — "it does not lie even when the delete failed" rather +# than "the delete is made certain" — and it is why the clear stays +# best-effort in the engine rather than growing a retry. +# +# The comparison is lexicographic on the two timestamps, which is exact +# because BOTH are written by the same engine in the same format (an ISO-8601 +# UTC instant). It is not a general date comparison and must not be reused as +# one. +# +# With no cycle stamp there is nothing to compare against, and the refusal is +# reported: an engine that has refused and never succeeded is exactly the case +# the record exists for. +_remote_sync_engine_refusal_current() { + local team="$1" file stamp last_cycle refused_at + file="$(_remote_sync_engine_refusal "$team")" + [ -f "$file" ] || return 0 + # Quiet on purpose: an unreadable record is ABSENT, and saying so on stderr + # would put a parse error into the same stream a caller reads the JSON from + # (measured — it turned the "unreadable reads as absent" case red). + refused_at="$(_remote_read_config_field "$file" '$.at' 2>/dev/null)" + [ -n "$refused_at" ] && [ "$refused_at" != "null" ] || return 0 + stamp="$(_remote_sync_engine_cycle_stamp "$team")" + if [ -f "$stamp" ]; then + last_cycle="$(_remote_read_config_field "$stamp" '$.last_success_at' 2>/dev/null)" + if [ -n "$last_cycle" ] && [ "$last_cycle" != "null" ] && [[ "$last_cycle" > "$refused_at" ]]; then + return 0 + fi + fi + cat "$file" 2>/dev/null || true +} + # _remote_holds_current_key -> 0 when this machine holds the identity # for the team's CURRENT epoch, 1 otherwise. # @@ -2154,9 +2196,11 @@ _remote_status_one() { # sentence invented here is wrong for some server. What it may add is where # the operator of that server would be reached, which is the host the # binding already names. - local refusal_file refusal_status refusal_code refusal_at refusal_host - refusal_file="$(_remote_sync_engine_refusal "$team")" - if [ -f "$refusal_file" ]; then + local refusal_raw refusal_status refusal_code refusal_at refusal_host + # Through the currency check, not straight off the file — see the helper. + refusal_raw="$(_remote_sync_engine_refusal_current "$team")" + if [ -n "$refusal_raw" ]; then + local refusal_file="$(_remote_sync_engine_refusal "$team")" refusal_status="$(_remote_read_config_field "$refusal_file" '$.status')" refusal_code="$(_remote_read_config_field "$refusal_file" '$.code')" refusal_at="$(_remote_read_config_field "$refusal_file" '$.at')" @@ -2247,9 +2291,10 @@ _remote_status_json_one() { # Passed as a whole file rather than as parsed fields: whatever the engine # recorded is what the reader gets, and adding a field there does not need a # change here. - local refusal_raw="" refusal_file - refusal_file="$(_remote_sync_engine_refusal "$team")" - [ -f "$refusal_file" ] && refusal_raw="$(cat "$refusal_file" 2>/dev/null || true)" + # Same currency check as the human line, from the same helper: the two must + # not be able to disagree about whether a refusal still stands. + local refusal_raw + refusal_raw="$(_remote_sync_engine_refusal_current "$team")" printf '%s' "$raw" | REFUSAL_JSON="$refusal_raw" python3 -c ' import json, os, sys team, engine_state, engine_pid_text = sys.argv[1:4] diff --git a/tests/test_remote_refusal.bats b/tests/test_remote_refusal.bats index f8237fc0..0e458f04 100644 --- a/tests/test_remote_refusal.bats +++ b/tests/test_remote_refusal.bats @@ -113,3 +113,58 @@ write_refusal() { # something in that file, or these three prove nothing. grep -q 'isRefusal' "$SCRIPTS/internal/remote-sync.mjs" } + +# ── a refusal that is no longer true ──────────────────────────────────────── +# +# The engine removes the record after a successful cycle, best-effort. If that +# removal fails — an unwritable run directory, a permission change, a crash +# between the two writes — the file stays. Nothing else stood between it and +# the operator, so `status` reported a reversed decision for ever (raised in +# review, and it contradicted this PR's own stated standard). +# +# The reader now compares the record to the last successful cycle. Deleting can +# fail; comparing cannot. + +write_cycle_stamp() { + printf '%s\n' "{\"type\":\"sync_cycle_stamp\",\"first_success_at\":\"$1\",\"last_success_at\":\"$1\"}" \ + > "$TEST_SKILL_DIR/run/remote-sync.testteam.cycles.json" +} + +@test "a refusal older than the last successful cycle is not reported, even if the file remains" { + write_refusal 402 payment_required # recorded at 2026-08-14T00:00:00Z + write_cycle_stamp "2026-08-14T01:00:00Z" # and a cycle succeeded after it + # The file is still there — this is the failed-delete case, made deliberate. + [ -f "$TEST_SKILL_DIR/run/remote-sync.testteam.refusal.json" ] + + run bash "$SCRIPTS/remote.sh" status testteam + [ "$status" -eq 0 ] + refute grep -q 'refused:' <<<"$output" + + run bash "$SCRIPTS/remote.sh" status testteam --json + local got + got="$(printf '%s' "$output" | python3 -c 'import json,sys; print(json.load(sys.stdin)["refusal"])')" + [ "$got" = "None" ] +} + +@test "a refusal newer than the last successful cycle is still reported" { + # The other side of the comparison. Without this the check could be satisfied + # by never reporting anything, which is the failure mode of every filter. + write_cycle_stamp "2026-08-13T00:00:00Z" + write_refusal 402 payment_required # recorded a day later + run bash "$SCRIPTS/remote.sh" status testteam + printf '%s' "$output" | grep -q 'refused: the server answered 402' + + run bash "$SCRIPTS/remote.sh" status testteam --json + local got + got="$(printf '%s' "$output" | python3 -c 'import json,sys; print(json.load(sys.stdin)["refusal"]["status"])')" + [ "$got" = "402" ] +} + +@test "with no cycle stamp at all, the refusal is reported" { + # An engine that has refused and never succeeded is exactly the case the + # record exists for. Nothing to compare against must not mean "assume stale". + write_refusal 402 payment_required + [ ! -f "$TEST_SKILL_DIR/run/remote-sync.testteam.cycles.json" ] + run bash "$SCRIPTS/remote.sh" status testteam + printf '%s' "$output" | grep -q 'refused: the server answered 402' +}