Skip to content

Commit ac1e121

Browse files
committed
test: gate daemon RPC wire compatibility against the last released tag (#1432)
ADR 0006 fixes exactly when DAEMON_RPC_PROTOCOL_VERSION must be bumped, and nothing checked that it was. The runtime guard (readRemoteDaemonHealth) refuses a mismatched peer, but only fires when someone remembered the bump — a wire change that skipped it left both sides advertising protocol 2 while parsing different payloads, which is the failure ADR 0006 exists to prevent. Local daemons cannot skew (isReusableDaemonInfo takes over on any package version mismatch). Cross-machine is skewed by design — proxy, cloud/limrun, a remote macOS host — and ADR 0006 explicitly rules package version out as the compatibility gate there, so the one boundary where skew is intended was the one boundary with no gate. test/wire-compat/surface.ts declares the wire surface grouped by the ADR bullet each group serves, quoting it, with an `uncovered` note where a bullet is only partly digestible (the /health and /rpc literals inside http-server.ts stay reviewer-owned: a moved route 404s at connect time rather than misparsing). ledger.json records what each declaration hashes to, at which protocol version. Two gates, split for the same reason the replay-compat corpus splits: - unit-core holds the ledger to its source and prints the digest to paste; - Released-Surface Compatibility reads the ledger at the last RELEASED tag and requires the drift since then to carry a bump or a compatibleChanges ack. From one commit a bumped ledger and an unbumped one are both just an edited file, so only a released baseline can tell them apart. Acks are keyed by the digest they cover, so one "added an optional field" cannot launder later changes. Digests ignore comments and formatting; the manifest's closure is derived from the AST, so a field typed by an unlisted sibling fails rather than sitting outside the gate. CI cost: one added job (checkout + toolchain + two node scripts, ~1 min), mirroring the existing full-history replay-compat job.
1 parent 4279d4c commit ac1e121

20 files changed

Lines changed: 1140 additions & 1 deletion

.github/workflows/ci.yml

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -232,6 +232,30 @@ jobs:
232232
- name: Verify corpus entries against their released blobs
233233
run: pnpm check:replay-compat
234234

235+
released-surface-compat:
236+
# The daemon RPC wire ledger (#1432) is compared against the ledger as it
237+
# stood at the last RELEASED tag, which only a full-history checkout can
238+
# read. Same split as the replay-compat corpus above: the shallow unit lane
239+
# holds the ledger to its source, this job holds it to the last release.
240+
name: Released-Surface Compatibility
241+
runs-on: ubuntu-latest
242+
timeout-minutes: 10
243+
steps:
244+
- name: Checkout
245+
uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2
246+
with:
247+
fetch-depth: 0
248+
fetch-tags: true
249+
250+
- name: Setup toolchain
251+
uses: ./.github/actions/setup-node-pnpm
252+
253+
- name: Verify the wire-compat rules
254+
run: pnpm check:daemon-wire-compat:test
255+
256+
- name: Compare the daemon RPC wire surface against the last released tag
257+
run: pnpm check:daemon-wire-compat
258+
235259
coverage:
236260
# Runs the full unit + provider-integration suites under coverage with
237261
# thresholds, so a separate unit-tests job would rerun the same tests.

CONTRIBUTING.md

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -72,6 +72,18 @@ when a running daemon needs to pick up a new TypeScript build.
7272
`pnpm package:npm` is a release guard, not a routine development command. Use the specific commands
7373
above while iterating.
7474

75+
### Released-surface baselines roll forward on publish
76+
77+
Compatibility gates baseline against the last **released tag**, not against `main`, so publishing is
78+
what advances them — there is no separate baseline-refresh step and no regenerate command. Tagging a
79+
release makes that commit's `test/wire-compat/ledger.json` the new baseline for
80+
`pnpm check:daemon-wire-compat`, and its `.ad` corpus tags the new ceiling for
81+
`pnpm check:replay-compat`. The practical consequence for a normal PR: wire churn *within* an
82+
unreleased branch is free, and only the net change since the last publish has to carry a
83+
`DAEMON_RPC_PROTOCOL_VERSION` bump or a `compatibleChanges` acknowledgment. After a release that
84+
bumped the protocol version, the acknowledgments accumulated against the previous one no longer
85+
match any current digest and are dropped — git history keeps the audit trail.
86+
7587
## Validate a change
7688

7789
Use the smallest trustworthy loop while editing:

docs/adr/0006-daemon-rpc-protocol-version.md

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -61,6 +61,15 @@ Protocol-breaking changes must update `DAEMON_RPC_PROTOCOL_VERSION`, tests that
6161
metadata, and at least one remote-client regression test that proves mismatched protocols fail before
6262
command RPC.
6363

64+
The bump rules above are enforced rather than remembered (#1432). `test/wire-compat/surface.ts`
65+
declares the wire surface grouped by the bullets in this decision, and `test/wire-compat/ledger.json`
66+
records what each declaration hashes to and at which protocol version. The unit lane fails when a
67+
declaration's shape moves without the ledger following, and `pnpm check:daemon-wire-compat` compares
68+
the ledger against the last released tag: drift since that release must carry either a bump or a
69+
`compatibleChanges` acknowledgment invoking the additive list above. Where a bullet is only partly
70+
digestible — the `/health` and `/rpc` path literals inside `http-server.ts` — the manifest records
71+
the gap and its reason instead of implying coverage. See `test/wire-compat/README.md`.
72+
6473
Legacy remote daemons without `rpcProtocolVersion` remain reachable. This keeps the first release of
6574
the proxy compatible with older HTTP daemons, but it means absence of the marker is not proof of
6675
compatibility.

docs/agents/testing.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,7 @@ The mapping it encodes, for when you need to run a gate directly or reason about
4040
| CLI help/guidance (`src/cli/parser/cli-help.ts`, `src/cli-schema/`) | `pnpm exec vitest run src/cli/parser/__tests__ src/cli-schema/command-schema-guards.test.ts scripts/__tests__` — the `scripts/__tests__` gates enforce help-topic benchmark coverage and pin the bench's quoted CLI samples to the real renderers |
4141
| Help benchmark cases (`scripts/help-conformance-*.mjs`) | `pnpm exec vitest run scripts/__tests__` (deterministic gates); model-backed: `pnpm bench:help-conformance` (paid LLM calls, local only) |
4242
| `.ad` grammar (`src/replay/script.ts`, gesture arity, replay vars) | `pnpm exec vitest run --project unit-core test/replay-compat` — the frozen replay-compat corpus asserts which released script surfaces still parse; a flipped verdict is edited in `test/replay-compat/manifest.ts`, never in the script. Adding or re-pinning a corpus entry also runs `pnpm check:replay-compat`, which re-derives each entry from its release tag in git history |
43+
| Daemon RPC wire surface (the declarations listed in `test/wire-compat/surface.ts` — JSON-RPC envelope, request/response/error/artifact/progress framing, `/health` payload, HTTP auth headers) | `pnpm exec vitest run --project unit-core test/wire-compat` holds the ledger to its source and prints the digest to paste; `pnpm check:daemon-wire-compat` compares it against the last released tag and requires a `DAEMON_RPC_PROTOCOL_VERSION` bump or a `compatibleChanges` ack for the drift. Read ADR 0006 to decide which; `test/wire-compat/README.md` walks both |
4344
| Anything in `src/`, `test/` | `pnpm format` (`skills/` is Markdown-only guidance: oxfmt ignores `**/*.md`, and the affected-check selector classifies it docs-only) |
4445
| Workspace package source (`packages/*/src/**`) | Root format/lint/typecheck plus layering (R11 package-boundaries); Vitest resolves affected tests through the module graph; package manifests/tsconfigs fail open to the full set |
4546
| A decision kernel or its tests (`packages/kernel/src/errors.ts`, `src/daemon/ref-frame.ts`, `src/commands/interaction/runtime/settle.ts`, `src/utils/scroll-edge-state.ts`, `packages/selectors/src/`) | `pnpm mutation:affected --base origin/main` (minutes; GitHub runs it per PR — see the mutation ratchet section) |

package.json

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -138,6 +138,8 @@
138138
"check:package": "node --experimental-strip-types scripts/check-package.ts",
139139
"check:command-docs": "vitest run --project unit-core src/__tests__/command-doc-coverage.test.ts",
140140
"check:replay-compat": "node --experimental-strip-types scripts/check-replay-compat-provenance.ts",
141+
"check:daemon-wire-compat": "node --experimental-strip-types scripts/wire-compat/run.ts",
142+
"check:daemon-wire-compat:test": "node --experimental-strip-types scripts/node-test-tmpdir.ts --experimental-strip-types --test scripts/wire-compat/model.test.ts",
141143
"check:tmpdir-leaks": "node --experimental-strip-types scripts/check-tmpdir-leaks.ts",
142144
"check:tmpdir-leaks:test": "node --experimental-strip-types scripts/node-test-tmpdir.ts --experimental-strip-types --test scripts/check-tmpdir-leaks-model.test.ts scripts/vitest-tmpdir-global-setup.test.ts scripts/node-test-tmpdir.test.ts scripts/swift-toolchain-tmpdir.test.ts",
143145
"check:freerange": "fr",

scripts/check-affected/checks.ts

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -167,6 +167,15 @@ export const CHECK_CATALOG: readonly CheckSpec[] = [
167167
ciJobs: ['Replay-Compat Provenance'],
168168
localRunnable: true,
169169
},
170+
{
171+
id: 'daemon-wire-compat',
172+
label: 'Daemon RPC wire surface vs. last released tag',
173+
kind: { type: 'script', script: 'check:daemon-wire-compat' },
174+
// Same shape as replay-compat: the released ledger is only readable from a
175+
// full-history checkout, so this cannot live in the shallow unit lane.
176+
ciJobs: ['Released-Surface Compatibility'],
177+
localRunnable: true,
178+
},
170179
];
171180

172181
export function getCheckSpec(id: CheckId): CheckSpec {

scripts/check-affected/model.test.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -223,6 +223,7 @@ test('every catalog command resolves against package scripts', () => {
223223
'build:macos-helper': 'x',
224224
'test:smoke:web': 'x',
225225
'check:replay-compat': 'x',
226+
'check:daemon-wire-compat': 'x',
226227
};
227228
for (const spec of CHECK_CATALOG) {
228229
const command = resolveCommand(spec, scripts, 'origin/main');

scripts/check-affected/model.ts

Lines changed: 34 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,8 @@
1919
// non-.ts fixture whose owning suite cannot be derived). Existing GitHub CI
2020
// remains authoritative; this only optimizes local/agent feedback.
2121

22+
import { WIRE_SURFACE_FILES } from '../../test/wire-compat/surface.ts';
23+
2224
export type CheckId =
2325
| 'format'
2426
| 'lint'
@@ -39,7 +41,8 @@ export type CheckId =
3941
| 'android-helpers'
4042
| 'macos-helper'
4143
| 'web-smoke'
42-
| 'replay-compat';
44+
| 'replay-compat'
45+
| 'daemon-wire-compat';
4346

4447
// The complete local check universe. A fail-open plan selects all of these;
4548
// keep it in sync with the catalog in checks.ts (asserted by the self-test).
@@ -66,6 +69,7 @@ export const ALL_CHECKS: readonly CheckId[] = [
6669
'macos-helper',
6770
'web-smoke',
6871
'replay-compat',
72+
'daemon-wire-compat',
6973
];
7074

7175
export type SelectionReason = {
@@ -331,6 +335,34 @@ const replayCompatOwnership: OwnershipRule = ({ file }) => {
331335
return selections;
332336
};
333337

338+
// The daemon RPC wire ledger (#1432). The wire SOURCE files are the ones that
339+
// would otherwise slip: editing `packages/kernel/src/contracts.ts` selects
340+
// vitest-related, but the wire gate reads that file as TEXT rather than
341+
// importing it, so it is invisible to the module graph Vitest walks. The file
342+
// list is read from the manifest instead of restated here, so a declaration
343+
// added under a new file selects the gate the day it is listed.
344+
//
345+
// `ledger.json` needs the rule for the second reason `.ad` corpus data does:
346+
// a non-.ts file under test/ resolves to `format` alone and would fail open.
347+
// (`scripts/wire-compat/` needs no branch — all of scripts/ already fails open.)
348+
const daemonWireCompatOwnership: OwnershipRule = ({ file }) => {
349+
if (!file.startsWith('test/wire-compat/') && !WIRE_SURFACE_FILES.includes(file)) return [];
350+
return [
351+
reason(
352+
'daemon-wire-compat',
353+
file,
354+
'own:daemon-wire-compat',
355+
'the wire ledger is compared against the last released tag',
356+
),
357+
reason(
358+
'unit',
359+
file,
360+
'own:daemon-wire-compat',
361+
'the wire ledger is held to its source by the unit-lane gate',
362+
),
363+
];
364+
};
365+
334366
const BUILD_OWNERSHIP: ReadonlyArray<{
335367
check: CheckId;
336368
rule: string;
@@ -392,6 +424,7 @@ const OWNERSHIP_RULES: readonly OwnershipRule[] = [
392424
nodeIntegrationOwnership,
393425
testAppOwnership,
394426
replayCompatOwnership,
427+
daemonWireCompatOwnership,
395428
buildOwnership,
396429
];
397430

scripts/check-affected/run.test.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -104,6 +104,7 @@ const ALL_SCRIPTS: Record<string, string> = {
104104
'test:integration:node': 'x',
105105
'test:integration:progress:check': 'x',
106106
'check:replay-compat': 'x',
107+
'check:daemon-wire-compat': 'x',
107108
};
108109

109110
const ARGS = { base: 'origin/main', head: 'HEAD', json: false, run: true };

scripts/wire-compat/model.test.ts

Lines changed: 127 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,127 @@
1+
/**
2+
* The acceptance criterion #1432 states in one sentence — "a wire-type change
3+
* without an RPC version bump fails" — proved from fixtures.
4+
*
5+
* It cannot be proved end-to-end yet: no released tag carries a wire ledger
6+
* until this lands and ships, so `run.ts` has nothing to diff against. These
7+
* cases pin the rule now, and `run.ts` becomes a thin git reader over them.
8+
*/
9+
10+
import assert from 'node:assert/strict';
11+
import test from 'node:test';
12+
import type { WireLedger } from '../../test/wire-compat/ledger.ts';
13+
import { compareWireLedgers } from './model.ts';
14+
15+
const RESPONSE = 'packages/kernel/src/contracts.ts#DaemonResponse';
16+
const META = 'packages/kernel/src/contracts.ts#DaemonRequestMeta';
17+
18+
function ledger(overrides: Partial<WireLedger> = {}): WireLedger {
19+
return {
20+
protocolVersion: 2,
21+
declarations: { [RESPONSE]: 'sha256:aaa', [META]: 'sha256:bbb' },
22+
compatibleChanges: [],
23+
...overrides,
24+
};
25+
}
26+
27+
function compare(current: WireLedger, digests: Record<string, string>) {
28+
return compareWireLedgers({
29+
baselineTag: 'v0.20.6',
30+
released: ledger(),
31+
current,
32+
digests: new Map(Object.entries(digests)),
33+
});
34+
}
35+
36+
test('an unchanged wire surface passes', () => {
37+
const result = compare(ledger(), { [RESPONSE]: 'sha256:aaa', [META]: 'sha256:bbb' });
38+
assert.deepEqual(result.failures, []);
39+
assert.deepEqual(result.changed, []);
40+
});
41+
42+
test('a changed wire declaration without a bump or ack fails, naming the symbol', () => {
43+
const current = ledger({ declarations: { [RESPONSE]: 'sha256:zzz', [META]: 'sha256:bbb' } });
44+
const result = compare(current, { [RESPONSE]: 'sha256:zzz', [META]: 'sha256:bbb' });
45+
assert.deepEqual(result.changed, [RESPONSE]);
46+
assert.equal(result.failures.length, 1);
47+
assert.match(result.failures[0]!, /DaemonResponse/);
48+
assert.match(result.failures[0]!, /without bumping DAEMON_RPC_PROTOCOL_VERSION \(still 2\)/);
49+
});
50+
51+
test('the same change passes once the protocol version is bumped', () => {
52+
const current = ledger({
53+
protocolVersion: 3,
54+
declarations: { [RESPONSE]: 'sha256:zzz', [META]: 'sha256:bbb' },
55+
});
56+
const result = compare(current, { [RESPONSE]: 'sha256:zzz', [META]: 'sha256:bbb' });
57+
assert.equal(result.bumped, true);
58+
assert.deepEqual(result.failures, []);
59+
});
60+
61+
test('the same change passes with a compatible-change ack at the new digest', () => {
62+
const current = ledger({
63+
declarations: { [RESPONSE]: 'sha256:zzz', [META]: 'sha256:bbb' },
64+
compatibleChanges: [
65+
{ declaration: RESPONSE, digest: 'sha256:zzz', rationale: 'Added an optional field.' },
66+
],
67+
});
68+
const result = compare(current, { [RESPONSE]: 'sha256:zzz', [META]: 'sha256:bbb' });
69+
assert.deepEqual(result.failures, []);
70+
});
71+
72+
// The ack is keyed by the digest it covers precisely so it expires. Without
73+
// this, one "added an optional field" ack would launder every later change to
74+
// the same declaration.
75+
test('an ack pinned to a superseded digest does not cover the next change', () => {
76+
const current = ledger({
77+
declarations: { [RESPONSE]: 'sha256:yyy', [META]: 'sha256:bbb' },
78+
compatibleChanges: [
79+
{ declaration: RESPONSE, digest: 'sha256:zzz', rationale: 'Covered the previous change.' },
80+
],
81+
});
82+
const result = compare(current, { [RESPONSE]: 'sha256:yyy', [META]: 'sha256:bbb' });
83+
assert.equal(result.failures.length, 1);
84+
assert.match(result.failures[0]!, /DaemonResponse/);
85+
});
86+
87+
test('an ack with an empty rationale does not count as an ack', () => {
88+
const current = ledger({
89+
declarations: { [RESPONSE]: 'sha256:zzz', [META]: 'sha256:bbb' },
90+
compatibleChanges: [{ declaration: RESPONSE, digest: 'sha256:zzz', rationale: ' ' }],
91+
});
92+
const result = compare(current, { [RESPONSE]: 'sha256:zzz', [META]: 'sha256:bbb' });
93+
assert.equal(result.failures.length, 1);
94+
});
95+
96+
test('a removed wire declaration fails even when acked, because only a bump covers it', () => {
97+
const current = ledger({
98+
declarations: { [META]: 'sha256:bbb' },
99+
compatibleChanges: [
100+
{ declaration: RESPONSE, digest: 'sha256:aaa', rationale: 'Nobody used it.' },
101+
],
102+
});
103+
const result = compare(current, { [META]: 'sha256:bbb' });
104+
assert.deepEqual(result.removed, [RESPONSE]);
105+
assert.equal(result.failures.length, 1);
106+
assert.match(result.failures[0]!, /an ack cannot cover it/);
107+
});
108+
109+
test('a removed wire declaration passes with a bump', () => {
110+
const current = ledger({ protocolVersion: 3, declarations: { [META]: 'sha256:bbb' } });
111+
const result = compare(current, { [META]: 'sha256:bbb' });
112+
assert.deepEqual(result.failures, []);
113+
});
114+
115+
test('a newly added wire declaration is additive and needs nothing', () => {
116+
const added = 'packages/kernel/src/contracts.ts#NewEnvelope';
117+
const current = ledger({
118+
declarations: { [RESPONSE]: 'sha256:aaa', [META]: 'sha256:bbb', [added]: 'sha256:ccc' },
119+
});
120+
const result = compare(current, {
121+
[RESPONSE]: 'sha256:aaa',
122+
[META]: 'sha256:bbb',
123+
[added]: 'sha256:ccc',
124+
});
125+
assert.deepEqual(result.added, [added]);
126+
assert.deepEqual(result.failures, []);
127+
});

0 commit comments

Comments
 (0)