From 5373c228b3241865669635ed81ac7e98f85ea863 Mon Sep 17 00:00:00 2001 From: Lucide <279298543+LucideLarp@users.noreply.github.com> Date: Sat, 25 Jul 2026 15:18:58 +0200 Subject: [PATCH] tests: say what the state/ canary does not catch It compares file length rather than content, and both driver suites run it against the one shared directory so either can be failed by the other's writes. Written beside it because the obvious tidy-up, watching only this file's own writes, would miss the bug it exists to catch. reconcile-subs.js no longer points at the retired local runner for the lock design. The reasoning it was citing stays. --- scripts/reconcile-subs.js | 9 ++++----- scripts/test/renew-invites-driver.test.js | 10 ++++++++++ scripts/test/subs-driver.test.js | 10 ++++++++++ 3 files changed, 24 insertions(+), 5 deletions(-) diff --git a/scripts/reconcile-subs.js b/scripts/reconcile-subs.js index 5a33dc0..8b73d30 100644 --- a/scripts/reconcile-subs.js +++ b/scripts/reconcile-subs.js @@ -85,9 +85,8 @@ function writeJson(file, obj) { // wedge enforcement shut: a lock is evidence of a live pass only while it is // young enough to be one. // -// A kernel lock (flock, as run-fulfill.sh takes via lockf) would be stronger, -// because the kernel drops it when the process dies and no staleness rule is -// needed at all. Node's standard library does not expose one and this engine +// A kernel lock (flock, as lockf(1) takes) would be stronger, because the +// kernel drops it when the process dies and no staleness rule is needed at all. Node's standard library does not expose one and this engine // takes no dependencies, so the staleness window below is the price of that. // It is set well above any plausible pass: the slowest thing here is one // GitHub call per invitation, and a pass long enough to exceed this would have @@ -104,8 +103,8 @@ const LOCK_MAX_AGE_MS = 30 * 60 * 1000; // current time, and the staleness rule that is supposed to break it instead // reads it as a pass that started seconds ago. Enforcement would then be wedged // on that machine for the full window, on the strength of a lock nobody holds. -// run-fulfill.sh already learned this and gitignores its own lock; both repos -// ignore `state/*.lock` for this one. +// The ops side learned this the hard way and gitignores its locks for exactly +// this reason; both repos ignore `state/*.lock` for this one. function lockPathFor(statePath) { return path.join(path.dirname(statePath), `.${path.basename(statePath)}.lock`); } diff --git a/scripts/test/renew-invites-driver.test.js b/scripts/test/renew-invites-driver.test.js index 7f93068..f2aa60f 100644 --- a/scripts/test/renew-invites-driver.test.js +++ b/scripts/test/renew-invites-driver.test.js @@ -27,6 +27,16 @@ const { scratchDir } = require('../test-support/scratch-dir.js'); // Renewal defaults --state to state/bots-state.json, which in a real ops repo is // LIVE entitlement data. A harness bug that forgets to redirect it would write // test revocations into it. Assert the repository's own state/ is untouched. +// +// Deliberately blunt, and worth keeping that way. Two things it does NOT do, +// said here so the next reader does not tidy them into a guard that catches +// less. It compares each file's LENGTH and not its content, so a rewrite that +// lands on the same size passes. And it is not alone: the other driver suite +// runs the same check, in its own process, against this one shared directory, +// so a stray write fails both and the file that reports it need not be the file +// that caused it. Both are the price of measuring the real directory, which is +// the one thing a harness bug cannot route around. A narrower check that +// watched only its own writes would miss exactly the bug this exists to catch. const REPO_STATE = path.join(__dirname, '..', '..', 'state'); function repoStateFingerprint() { if (!fs.existsSync(REPO_STATE)) return 'absent'; diff --git a/scripts/test/subs-driver.test.js b/scripts/test/subs-driver.test.js index 007aed1..35e98e8 100644 --- a/scripts/test/subs-driver.test.js +++ b/scripts/test/subs-driver.test.js @@ -23,6 +23,16 @@ const lockPathFor = (statePath) => // defaults --state and --bots-state to paths under the working directory, so a // harness that forgets to redirect them silently commits test data, or worse, // edits live ops state. This asserts the repo's state/ is untouched by the run. +// +// Deliberately blunt, and worth keeping that way. Two things it does NOT do, +// said here so the next reader does not tidy them into a guard that catches +// less. It compares each file's LENGTH and not its content, so a rewrite that +// lands on the same size passes. And it is not alone: the other driver suite +// runs the same check, in its own process, against this one shared directory, +// so a stray write fails both and the file that reports it need not be the file +// that caused it. Both are the price of measuring the real directory, which is +// the one thing a harness bug cannot route around. A narrower check that +// watched only its own writes would miss exactly the bug this exists to catch. const REPO_STATE = path.join(__dirname, '..', '..', 'state'); function repoStateFingerprint() { if (!fs.existsSync(REPO_STATE)) return 'absent';