Skip to content

Commit 61c8df8

Browse files
committed
docs(sync): document drift guard third-round fix — strip autoNumber runtime counter from typeOptions hash (engine 2e)
CLAUDE.md: drift guard now hashes typeOptions through `fingerprintTypeOptions()`, which strips `maxUsedAutoNumber` — a read-only server counter advanced on every row creation, including by sync's own records phase. Hashing it raw (engine `2d`) made an unchanged schema abort with phantom `DRIFT` when any row landed between plan and apply, or when re-applying after a records run that
1 parent ab6749a commit 61c8df8

5 files changed

Lines changed: 100 additions & 8 deletions

File tree

CLAUDE.md

Lines changed: 1 addition & 1 deletion
Large diffs are not rendered by default.

packages/mcp-server/CHANGELOG.md

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,16 @@ External review (PR #19) found both, with a reproduction. Sync's drift guard is
99
two holes that compounded: the fingerprint usually could not see the change, and once a run was
1010
resuming it stopped looking.
1111

12+
- **Third review round (same day): `2d` hashed autoNumber's runtime counter — fixed as engine `2e`.**
13+
`typeOptions.maxUsedAutoNumber` is a read-only counter the server advances whenever a row is
14+
created — including by this sync's OWN records phase — so hashing it raw made an unchanged
15+
schema abort with a phantom `DRIFT` (review repro: counter 7 → 8, apply aborted), and a
16+
re-apply after any records run could trip its own guard. The fingerprint now strips it via
17+
`fingerprintTypeOptions()`, the same strip `apply.js` already performs before every write;
18+
semantic `typeOptions` changes on the same field still drift. Regressions cover the synthetic
19+
shape, the `normalizeSchema` parity path, and a full plan→apply run with only the counter
20+
advanced. Plans saved by `2d` abort `PLAN_STALE` with a re-plan instruction. Fails safe
21+
either way — the bug refused legitimate applies; it never overwrote anything.
1222
- **Second review round (same day): the widening itself had three defects, all fixed as engine `2d`.**
1323
(1) The fingerprint hashed `f.options` — a key NO normalized snapshot carries; the real key is
1424
`typeOptions` (`normalizeSchema`), so the widening was inert in production while its tests
@@ -45,8 +55,8 @@ resuming it stopped looking.
4555
- **A plan from an older engine aborts with `PLAN_STALE`, not `DRIFT`.** An older plan hashed
4656
fewer facets, so its digest can never match a current one. Reporting that as `DRIFT` blamed a
4757
collaborator; mid-resume it invited the user to wave an overwrite through to fix what was really
48-
a version mismatch. `ENGINE_VERSION` is exported (now `2d` — see the second-round entry above),
49-
so test fixtures track it instead of pinning a literal that goes stale on the next bump.
58+
a version mismatch. `ENGINE_VERSION` is exported (now `2e` — see the rounds above), so test
59+
fixtures track it instead of pinning a literal that goes stale on the next bump.
5060

5161
### Changed (2026-07-30 — `mirror` no longer claims more than it does)
5262

packages/mcp-server/src/sync/index.js

Lines changed: 24 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -18,9 +18,29 @@ import { writeSyncJobStatus, readSyncJobStatus } from './job-status.js';
1818
// passed on synthetic shapes — external review caught it with a live reproduction. 2d hashes
1919
// `typeOptions` and also folds in table SECTIONS (mirror prune deletes dest-only sections by
2020
// id, so a renamed section must read as drift, not get deleted under its old identity).
21-
// A plan saved by an older engine hashed LESS, so its fingerprint can never match a current
22-
// one — apply detects that by version and says "re-plan", instead of blaming a collaborator.
23-
export const ENGINE_VERSION = '2d';
21+
// '2e' exists because 2d hashed typeOptions RAW, including autoNumber's maxUsedAutoNumber —
22+
// a read-only runtime counter Airtable advances whenever a row is created, including by this
23+
// sync's OWN records phase. Any row landing between plan and apply (or a re-apply after a
24+
// records run) produced a phantom DRIFT abort on an unchanged schema. 2e strips it, exactly
25+
// as apply.js already strips it before every write.
26+
// A plan saved by an older engine hashed a DIFFERENT basis, so its fingerprint can never match
27+
// a current one — apply detects that by version and says "re-plan", instead of blaming a
28+
// collaborator.
29+
export const ENGINE_VERSION = '2e';
30+
31+
/**
32+
* typeOptions as hashed by the drift fingerprint: schema SEMANTICS only, runtime state out.
33+
*
34+
* `maxUsedAutoNumber` is the one known runtime key — a counter the server advances on row
35+
* creation, not something a collaborator edits. apply.js strips it at both of its write
36+
* sites for the same reason (the API rejects it); if another runtime key ever surfaces,
37+
* add it HERE and THERE together, and bump ENGINE_VERSION.
38+
*/
39+
function fingerprintTypeOptions(typeOptions) {
40+
if (!typeOptions || typeof typeOptions !== 'object') return typeOptions ?? null;
41+
const { maxUsedAutoNumber, ...semantic } = typeOptions;
42+
return semantic;
43+
}
2444

2545
/**
2646
* Stable JSON — object keys sorted at every depth.
@@ -74,7 +94,7 @@ export function fingerprintSchema(snap, { includeViewConfig = false } = {}) {
7494
// must construct fields the way normalizeSchema does.
7595
.map((t) => `${t.id}:${t.name}:`
7696
+ t.fields.map((f) =>
77-
`${f.id}=${f.name}=${f.type}=${stableJson(f.typeOptions ?? null)}=${f.description ?? ''}`
97+
`${f.id}=${f.name}=${f.type}=${stableJson(fingerprintTypeOptions(f.typeOptions))}=${f.description ?? ''}`
7898
).sort().join(',')
7999
+ ';V:' + (t.views || []).map((v) =>
80100
`${v.id}=${v.name}=${v.type}` + (includeViewConfig ? `=${stableJson(v.config ?? null)}` : '')

packages/mcp-server/test/sync/test-index.test.js

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -103,6 +103,34 @@ describe('sync index.plan', () => {
103103
assert.equal(fingerprintSchema(a), fingerprintSchema(b), 'key order must not be drift');
104104
});
105105

106+
it('fingerprintSchema: autoNumber maxUsedAutoNumber is runtime state, NOT drift', () => {
107+
// The counter advances whenever a row is created — including by this sync's OWN records
108+
// phase — so hashing it made an unchanged schema abort with a phantom DRIFT (review repro:
109+
// counter 7 → 8, fingerprint changed, apply aborted). apply.js strips it before every
110+
// write for the same reason; the fingerprint must share that knowledge.
111+
const mk = (typeOptions) => ({ tables: [{ id: 't1', name: 'T', fields: [
112+
{ id: 'f1', name: 'ID', type: 'autoNumber', typeOptions },
113+
] }] });
114+
assert.equal(
115+
fingerprintSchema(mk({ maxUsedAutoNumber: 7 })),
116+
fingerprintSchema(mk({ maxUsedAutoNumber: 8 })),
117+
'a counter-only change must NOT be drift',
118+
);
119+
// …but a SEMANTIC typeOptions change on the same field still is.
120+
assert.notEqual(
121+
fingerprintSchema(mk({ maxUsedAutoNumber: 7, format: 'A' })),
122+
fingerprintSchema(mk({ maxUsedAutoNumber: 8, format: 'B' })),
123+
'a real typeOptions change must still be drift even with the counter moving',
124+
);
125+
// And through the REAL normalizer, same asymmetry.
126+
const raw = (n, fmt) => ({ data: { tableSchemas: [{
127+
id: 'tbl1', name: 'T', primaryColumnId: 'fld1',
128+
columns: [{ id: 'fld1', name: 'ID', type: 'autoNumber', typeOptions: { maxUsedAutoNumber: n, ...(fmt ? { format: fmt } : {}) } }],
129+
}] } });
130+
assert.equal(fingerprintSchema(normalizeSchema(raw(7))), fingerprintSchema(normalizeSchema(raw(8))));
131+
assert.notEqual(fingerprintSchema(normalizeSchema(raw(7, 'A'))), fingerprintSchema(normalizeSchema(raw(8, 'B'))));
132+
});
133+
106134
it('fingerprintSchema: a section rename is drift in the DEFAULT mode (mirror prune deletes by id)', () => {
107135
// pruneSchema deletes dest-only sections by ID. If a section renamed between plan and apply
108136
// does not read as drift, apply deletes it under its old identity — reproduced by review:

packages/mcp-server/test/sync/test-resume-drift.test.js

Lines changed: 35 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,8 @@ import { mkdtempSync } from 'node:fs';
1111
import { join } from 'node:path';
1212
import { tmpdir } from 'node:os';
1313
import { MockClient } from './helpers/mock-client.js';
14-
import { apply, applyJob, ENGINE_VERSION } from '../../src/sync/index.js';
14+
import { apply, applyJob, ENGINE_VERSION, fingerprintSchema } from '../../src/sync/index.js';
15+
import { normalizeSchema } from '../../src/sync/snapshot.js';
1516
import { savePlan, saveIdmap } from '../../src/sync/idmap.js';
1617
import { newJournal, recordDone, recordFailed, saveJournal } from '../../src/sync/journal.js';
1718

@@ -60,6 +61,39 @@ describe('sync index.apply — journal resume bypasses the drift guard', () => {
6061
assert.deepEqual(names, ['TableA'], `no action may run on an aborted resume: ${names}`);
6162
});
6263

64+
// Review reproduction, end to end: the ONLY change between plan and apply is autoNumber's
65+
// maxUsedAutoNumber advancing (7 → 8) — a counter the server bumps on row creation, including
66+
// by this sync's own records phase. Engine 2d hashed it and aborted DRIFT on an unchanged
67+
// schema; a re-apply after any records run would trip its own guard.
68+
it('an advanced maxUsedAutoNumber counter alone does NOT abort apply with DRIFT', async () => {
69+
process.env.AIRTABLE_USER_MCP_HOME = mkdtempSync(join(tmpdir(), 'resume-drift-counter-'));
70+
const client = new MockClient();
71+
// Seed a dest table with an autoNumber field, counter at 7.
72+
client.tables.push({
73+
id: 'tblD1', name: 'T', primaryColumnId: 'fldD1',
74+
columns: [{ id: 'fldD1', name: 'ID', type: 'autoNumber', typeOptions: { maxUsedAutoNumber: 7 }, description: null }],
75+
views: [], rows: [],
76+
});
77+
// Plan against the CURRENT dest state (counter 7), no actions — pure drift-guard exercise.
78+
// normalizeSchema takes the FULL response ({data:{tableSchemas}}), same as snapshotBase does.
79+
const destAtPlanTime = normalizeSchema(await client.getApplicationData(DEST));
80+
savePlan(SRC, DEST, {
81+
planId: 'plnCTR', engineVersion: ENGINE_VERSION,
82+
destFingerprint: fingerprintSchema({ baseId: DEST, ...destAtPlanTime }),
83+
sourceBaseId: SRC, destBaseId: DEST,
84+
idmap: { tables: {}, fields: {}, views: {} },
85+
actions: [], orphans: [], warnings: [],
86+
});
87+
saveIdmap(SRC, DEST, { tables: {}, fields: {}, views: {}, records: {}, attachments: {} });
88+
89+
// Airtable advances the counter before apply runs.
90+
client.tables[client.tables.length - 1].columns[0].typeOptions.maxUsedAutoNumber = 8;
91+
92+
const out = await apply({ client, sourceBaseId: SRC, destBaseId: DEST, planId: 'plnCTR', runStartedAt: 't1' });
93+
assert.notEqual(out.machine.aborted, true,
94+
`a counter-only change must not read as drift: ${out.human}`);
95+
});
96+
6397
// A plan from an older engine hashed fewer facets, so its digest can NEVER match. Reporting that
6498
// as DRIFT blames a collaborator; reporting it mid-resume as RESUME_DRIFT invites the user to
6599
// wave through an overwrite to fix what is really a version mismatch.

0 commit comments

Comments
 (0)