Skip to content

Commit 0b444db

Browse files
ARHAEEMclaude
andcommitted
fix(sync): sort fields before hashing in fingerprintSchema (27)
fingerprintSchema sorted tables and views before hashing but not fields, on the same expression. Field-listing order isn't a stable API contract (and CLAUDE.md already classifies field order as best-effort, non-drift), so a field-order-only difference between plan-time and apply-time snapshots flipped the fingerprint. The apply() drift guard treats that as a real schema change: it hard-aborts a fresh apply (forcing a wasteful re-plan on an unchanged schema) or, on a resume, emits a false RESUME_DRIFT_BYPASS "someone else changed the dest" warning. Sort the composed id=name=type strings the same way the views line already does — equivalent to sorting by field id (unique, always the first token) since ids can never collide, so order stops mattering while a genuine rename/retype still changes the string content and therefore the hash. Swept src/sync/ for sibling instances of the same pattern (identity hash over an unsorted collection) — none found; every other join/sort site is either already order-independent or deliberately order-sensitive for best-effort order-drift reporting (compare.js). Added regression coverage: same fields in different order -> same fingerprint; add/remove/rename/retype a field -> different fingerprint. Reverted the fix locally to confirm the new test fails without it, then restored it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 parent fe12bb2 commit 0b444db

2 files changed

Lines changed: 57 additions & 2 deletions

File tree

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

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -16,14 +16,20 @@ const ENGINE_VERSION = '2b';
1616

1717
/**
1818
* Produce a deterministic SHA-256 fingerprint of a schema snapshot.
19-
* Order-independent: tables are sorted before hashing.
19+
* Order-independent: tables, fields, and views are all sorted before hashing — field/view/
20+
* column order is explicitly a "best-effort" (non-drift) concern elsewhere in the sync engine
21+
* (see compare.js), and Airtable's field-listing order is not guaranteed stable between two
22+
* reads of an otherwise-unchanged schema. Sorting the composed `id=name=type` strings is
23+
* equivalent to sorting by `id` (a unique, stable-per-field identifier that always appears
24+
* first in the string, before any content that legitimately changes the fingerprint like a
25+
* rename or retype) while keeping the same shape as the views line below.
2026
*
2127
* @param {{ tables: Array<{id:string, name:string, fields:Array<{id:string,name:string,type:string}>}> }} snap
2228
* @returns {string} hex digest
2329
*/
2430
export function fingerprintSchema(snap) {
2531
const basis = snap.tables
26-
.map((t) => `${t.id}:${t.name}:` + t.fields.map((f) => `${f.id}=${f.name}=${f.type}`).join(',')
32+
.map((t) => `${t.id}:${t.name}:` + t.fields.map((f) => `${f.id}=${f.name}=${f.type}`).sort().join(',')
2733
+ ';V:' + (t.views || []).map((v) => `${v.id}=${v.name}=${v.type}`).sort().join(','))
2834
.sort()
2935
.join('|');

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

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,55 @@ describe('sync index.plan', () => {
2020
const b = { tables: [{ id: 't2', name: 'B', fields: [] }, { id: 't1', name: 'A', fields: [{ id: 'f1', name: 'x', type: 'text' }] }] };
2121
assert.equal(fingerprintSchema(a), fingerprintSchema(b));
2222
});
23+
// Regression test (Sentry finding, Task 27): fields were NOT sorted before hashing while
24+
// tables and views were — a field-order-only difference between two snapshots of an
25+
// otherwise-identical schema (Airtable's field-listing order is not a stable/guaranteed
26+
// contract; CLAUDE.md classifies field/view/column order as "best-effort", i.e. non-drift)
27+
// flipped the fingerprint, which the apply() drift guard (index.js) treats as a real
28+
// schema change — aborting mode=apply (or, on a resume, emitting a scary but false
29+
// RESUME_DRIFT_BYPASS "someone else changed the dest" warning) on an unchanged schema.
30+
it('fingerprintSchema: reordering a table\'s fields (no content change) does NOT change the fingerprint', () => {
31+
const mkTable = (fields) => ({ id: 't1', name: 'T', fields, views: [{ id: 'v1', name: 'Grid view', type: 'grid' }] });
32+
const fA = { id: 'f1', name: 'Name', type: 'text' };
33+
const fB = { id: 'f2', name: 'Count', type: 'number' };
34+
const fC = { id: 'f3', name: 'Status', type: 'singleSelect' };
35+
const a = { tables: [mkTable([fA, fB, fC])] };
36+
const b = { tables: [mkTable([fC, fA, fB])] };
37+
const c = { tables: [mkTable([fB, fC, fA])] };
38+
assert.equal(fingerprintSchema(a), fingerprintSchema(b), 'permutation 1 must match');
39+
assert.equal(fingerprintSchema(a), fingerprintSchema(c), 'permutation 2 must match');
40+
});
41+
it('fingerprintSchema: a genuine field change (add/remove/rename/retype) still changes the fingerprint', () => {
42+
const base = { tables: [{ id: 't1', name: 'T', fields: [
43+
{ id: 'f1', name: 'Name', type: 'text' },
44+
{ id: 'f2', name: 'Count', type: 'number' },
45+
] }] };
46+
const fpBase = fingerprintSchema(base);
47+
48+
const added = { tables: [{ id: 't1', name: 'T', fields: [
49+
{ id: 'f1', name: 'Name', type: 'text' },
50+
{ id: 'f2', name: 'Count', type: 'number' },
51+
{ id: 'f3', name: 'Extra', type: 'text' },
52+
] }] };
53+
assert.notEqual(fingerprintSchema(added), fpBase, 'added field must change the fingerprint');
54+
55+
const removed = { tables: [{ id: 't1', name: 'T', fields: [
56+
{ id: 'f1', name: 'Name', type: 'text' },
57+
] }] };
58+
assert.notEqual(fingerprintSchema(removed), fpBase, 'removed field must change the fingerprint');
59+
60+
const renamed = { tables: [{ id: 't1', name: 'T', fields: [
61+
{ id: 'f1', name: 'Name', type: 'text' },
62+
{ id: 'f2', name: 'Renamed', type: 'number' },
63+
] }] };
64+
assert.notEqual(fingerprintSchema(renamed), fpBase, 'renamed field must change the fingerprint');
65+
66+
const retyped = { tables: [{ id: 't1', name: 'T', fields: [
67+
{ id: 'f1', name: 'Name', type: 'text' },
68+
{ id: 'f2', name: 'Count', type: 'singleLineText' },
69+
] }] };
70+
assert.notEqual(fingerprintSchema(retyped), fpBase, 'retyped field must change the fingerprint');
71+
});
2372
});
2473

2574
describe('sync index.plan — direction param (Task 10)', () => {

0 commit comments

Comments
 (0)