diff --git a/.agents/rules/no-control-bytes-in-source.mdc b/.agents/rules/no-control-bytes-in-source.mdc new file mode 100644 index 00000000..1e722e62 --- /dev/null +++ b/.agents/rules/no-control-bytes-in-source.mdc @@ -0,0 +1,66 @@ +--- +description: A NUL byte (0x00) and other control characters are reserved sentinels — control signals with defined meanings across the entire computing stack (NUL is THE string terminator), not data. Never put one in source, and never, ever use one as a separator or delimiter. It silently truncates or corrupts in every downstream system it flows through — C strings, DB drivers, serializers, logs, terminals, transports, git. Compose keys with a data structure, not a delimited string. +alwaysApply: false +globs: ["**/*"] +--- + +# Control bytes are signals, not data — never use one as a separator + +A NUL byte (`0x00`, `\0`) is not an unused character you can borrow. It is **the +string terminator** — a reserved sentinel with a defined meaning across the +whole computing stack: C strings, POSIX syscalls, filenames, most database +drivers, wire protocols, terminals, and git all treat NUL as "end of string" or +"end of record." The other C0 control characters (`0x00`–`0x1F` except +tab/newline/carriage-return, and `0x7F`) are signals too, not text. + +Using one as data — most often as a **separator** — is the specific mistake this +rule exists to stop. + +## Why the "it can't collide" reasoning is exactly backwards + +The tempting logic is: "a method name / a UUID / a path can't contain a NUL, so +it's a collision-proof delimiter." That property is real, and it is the whole +problem. **A NUL is collision-free in your data precisely because every +correctly-built system refuses to put one there.** You are not finding a clever +unused byte; you are being the one component that violates an invariant the +entire rest of the stack relies on. + +And the byte does not stay in your code. It flows downstream, where its reserved +meaning takes over and silently corrupts: + +- **C-string boundaries** (and anything backed by them) truncate at the NUL — + your `"a\0b"` becomes `"a"`, losing data with no error. +- **Database drivers, serializers, and wire protocols** reject it, truncate it, + or mangle it — often only in production, only for some inputs. +- **Logs, terminals, and text tooling** (`grep`, editors, `less`) misrender or + drop it, so the corruption is invisible while you debug. +- **git** classifies any file containing a NUL as binary, so the diff renders as + `Binary file not shown` and the file becomes unreviewable — one concrete + instance of the general breakage, not the reason. + +None of these are worth a delimiter. There is no gain that offsets shipping a +reserved sentinel into data. + +## What to do instead + +**Compose with a data structure, not a delimited string.** Reaching for +`` `${a}${b}` `` to key a `Map` by a tuple is the smell that invites a +"clever" separator. Use a shape that needs no delimiter at all: + +- Nested maps: `Map>`. +- A `Set`/`Map` of entry objects that each carry their own fields + (`{ a, b, ... }`), so nothing is re-parsed out of a joined key. +- Identity keys when reference semantics fit. + +**If a delimiter is genuinely unavoidable**, use an ordinary printable character +that cannot appear in either part, and say in a comment why it cannot collide. A +control byte is never the answer — not raw, and not as the `\0` escape, which +produces the same byte at runtime and signals the same broken instinct. + +## Detecting it + +```bash +git diff --numstat ...HEAD # a text file showing "- -" is binary to git +file path/to/suspect.ts # "data" instead of "… text" means a bad byte +LC_ALL=C grep -naP '[\x00-\x08\x0b\x0c\x0e-\x1f\x7f]' path/to/file # locate control bytes +``` diff --git a/packages/0-framework/2-authoring/service-rpc/src/serve.ts b/packages/0-framework/2-authoring/service-rpc/src/serve.ts index d1deab4f..8aee5c96 100644 --- a/packages/0-framework/2-authoring/service-rpc/src/serve.ts +++ b/packages/0-framework/2-authoring/service-rpc/src/serve.ts @@ -112,9 +112,16 @@ export const REPLAY_CACHE_MAX_ENTRIES = 1000; /** How long a completed 2xx/4xx answer stays replayable for a repeated key. */ const REPLAY_TTL_MS = 60_000; -type CacheEntry = - | { readonly kind: 'pending'; readonly promise: Promise } - | { readonly kind: 'completed'; readonly outcome: Outcome; readonly completedAt: number }; +interface CompletedEntry { + readonly kind: 'completed'; + readonly outcome: Outcome; + readonly completedAt: number; + // The entry knows where it lives, so eviction can delete it from byMethod + // without reconstructing its location. + readonly method: string; + readonly key: string; +} +type CacheEntry = { readonly kind: 'pending'; readonly promise: Promise } | CompletedEntry; /** * Per-method, per-key deduplication. A duplicate arriving mid-execution @@ -124,8 +131,9 @@ type CacheEntry = */ class IdempotencyStore { private readonly byMethod = new Map>(); - // LRU order over completed entries; Map insertion order is the list, so re-inserting moves a key to the end. - private readonly lruOrder = new Map(); + // Completed entries in least-recently-used order. A Set keeps insertion + // order, so deleting then re-adding an entry moves it to the newest end. + private readonly lru = new Set(); async dispatch(method: string, key: string, run: () => Promise): Promise { const bucket = this.bucketFor(method); @@ -136,11 +144,12 @@ class IdempotencyStore { } if (existing?.kind === 'completed') { if (Date.now() - existing.completedAt < REPLAY_TTL_MS) { - this.touch(method, key); + this.lru.delete(existing); + this.lru.add(existing); return existing.outcome; } bucket.delete(key); - this.lruOrder.delete(this.lruKey(method, key)); + this.lru.delete(existing); } const promise = run(); @@ -157,8 +166,16 @@ class IdempotencyStore { if (result.status >= 500) { bucket.delete(key); // retryable outcome — a retry must re-execute } else { - bucket.set(key, { kind: 'completed', outcome: result, completedAt: Date.now() }); - this.touch(method, key); + const entry: CompletedEntry = { + kind: 'completed', + outcome: result, + completedAt: Date.now(), + method, + key, + }; + bucket.set(key, entry); + this.lru.add(entry); + this.evictOverflow(); } return result; } @@ -172,23 +189,12 @@ class IdempotencyStore { return bucket; } - private lruKey(method: string, key: string): string { - return `${method}\0${key}`; - } - - /** Marks (method, key) most-recently-used, evicting the oldest completed entry once over the bound. */ - private touch(method: string, key: string): void { - const lruKey = this.lruKey(method, key); - this.lruOrder.delete(lruKey); - this.lruOrder.set(lruKey, { method, key }); - - if (this.lruOrder.size > REPLAY_CACHE_MAX_ENTRIES) { - const oldestKey = this.lruOrder.keys().next().value; - const oldest = oldestKey === undefined ? undefined : this.lruOrder.get(oldestKey); - if (oldestKey !== undefined && oldest !== undefined) { - this.lruOrder.delete(oldestKey); - this.byMethod.get(oldest.method)?.delete(oldest.key); - } + private evictOverflow(): void { + if (this.lru.size <= REPLAY_CACHE_MAX_ENTRIES) return; + const oldest = this.lru.values().next().value; + if (oldest !== undefined) { + this.lru.delete(oldest); + this.byMethod.get(oldest.method)?.delete(oldest.key); } } }