Skip to content

Commit 1febddc

Browse files
committed
fix(drift): resolve phantom both-diverged issue by canonicalizing drift checks
This commit addresses a bug where untouched resources would incorrectly report a `both-diverged` state due to stale `lastPulledHash` values. The solution involves extracting canonicalization logic into a new `canonical.ts` module, which is now shared across `pull.ts`, `push.ts`, `audit.ts`, and `drift.ts`. Key changes include: - Implementing a unified `canonicalizeForHash` function to ensure consistent hashing across all resource types. - Modifying the `checkDriftForUpdate` function to treat local and platform hashes as equivalent when they match, regardless of the stale baseline. - Adding regression tests to verify that pushes succeed without drift detection when local and dashboard resources are byte-identical. This fix prevents unnecessary push blocks and improves the overall reliability of the push process. All tests pass, confirming the stability of the changes.
1 parent be1fafb commit 1febddc

10 files changed

Lines changed: 742 additions & 296 deletions

improvements.md

Lines changed: 77 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1079,6 +1079,83 @@ RESOLVED 2026-06-03 (#TBD — PR number updates when opened).
10791079

10801080
---
10811081

1082+
## 23. Phantom `both-diverged`: canonicalization trapped in pull.ts + a gate that blocked when local and dashboard already agreed
1083+
1084+
**[RESOLVED 2026-06-03] (#TBD)**
1085+
1086+
**Discovered:** a scoped `npm run push` of an unedited assistant blocked with
1087+
`❌ drift detected ... [both-diverged]` and "Applied 0 resource(s)", even
1088+
though the local file matched the dashboard. A first fix looked like it
1089+
worked — but it was only ever verified with `--dry-run`, which **skips the
1090+
drift check entirely**, so the real push path was never exercised. The block
1091+
came back on the first real push.
1092+
1093+
### Problem (two layers)
1094+
1095+
**Layer 1 — canonicalization was trapped in `pull.ts` (architectural smell).**
1096+
`canonicalizeForHash` (the single basis pull writes `lastPulledHash` in:
1097+
resourceId refs, credential names, prompt-in-body, `_platformDefault` marker)
1098+
lived in `pull.ts`. Because `pull.ts` imports `drift.ts`, `drift.ts` could not
1099+
import it back (cycle) — so it grew a **divergent duplicate**
1100+
(`stripServerFields` / `SERVER_FIELDS`, byte-identical to pull's
1101+
`cleanResource` / `EXCLUDED_FIELDS`) plus a dead `hashPlatformResource`. The
1102+
first fix bridged the gap with an injected `hashPlatformPayload` callback +
1103+
manual hash plumbing at the push call site — a patch around the module
1104+
placement, not the root cause.
1105+
1106+
**Layer 2 — the real phantom (the bug the patch never reached).** Even with
1107+
matching bases, an untouched resource blocked because the **gate** treated a
1108+
stale baseline as a conflict. A diagnostic on the live resource:
1109+
1110+
```
1111+
hashLocalResource (local file) : 54734d2b…
1112+
platformHash (canonicalized remote) : 54734d2b… ← byte-identical
1113+
state lastPulledHash : 3a83baba… ← stale (older basis)
1114+
```
1115+
1116+
Local and dashboard **agreed perfectly** — nothing to reconcile. But
1117+
`classifyDrift` returns `both-diverged` whenever both live sides differ from
1118+
the baseline (a deliberate, test-pinned *descriptive* contract), and
1119+
`checkDriftForUpdate` only short-circuited on `platformHash === lastPulledHash`.
1120+
So a stale baseline (every resource in a freshly-upgraded customer repo, whose
1121+
`lastPulledHash` was written in an older hash basis) manufactured a conflict
1122+
where none existed.
1123+
1124+
### Risk
1125+
1126+
Push/apply unusable for normal resources without `--overwrite` — the exact
1127+
escape hatch that silently clobbers concurrent dashboard edits. The phantom
1128+
drift trained operators to reach for the dangerous flag on every push.
1129+
1130+
### Resolution
1131+
1132+
1. **Extracted `src/canonical.ts`** (dependency-light: credentials + types).
1133+
It holds `VapiResource`, `EXCLUDED_FIELDS`, `cleanResource`,
1134+
`buildReverseMap`, `resolveReferencesToResourceIds`, and
1135+
`canonicalizeForHash`. `pull.ts`, `push.ts`, `audit.ts`, AND `drift.ts` now
1136+
import the ONE definition. Deleted drift's duplicate field-strip and the
1137+
dead `hashPlatformResource`. `checkDriftForUpdate` computes platform + local
1138+
hashes itself (no injected callback, no hash plumbing at the call site).
1139+
2. **Fixed the gate, not the classifier.** `checkDriftForUpdate` now returns
1140+
`match` (no-op, never blocks) when `localHash === platformHash`, regardless
1141+
of the baseline. `classifyDrift` keeps its descriptive `both-diverged`
1142+
contract (it still signals the stale pointer) — policy lives in the gate.
1143+
1144+
Regression coverage: `tests/push-stale-baseline-noop.test.ts` (e2e — stale
1145+
baseline + local==dashboard → push applies, no block) and `tests/drift.test.ts`
1146+
(canonicalization resolves tool UUID→slug so a clean resource matches baseline).
1147+
1148+
### Lesson
1149+
1150+
`--dry-run` is NOT a verification of the drift path — it skips it. Verify
1151+
push-gate changes with a real (idempotent) push or the e2e harness.
1152+
1153+
### Status
1154+
1155+
RESOLVED 2026-06-03 (#TBD — PR number updates when opened).
1156+
1157+
---
1158+
10821159
## Out of scope (intentionally not improvements)
10831160

10841161
- **State file is identity-only and not git-ignored.** It's intentionally

src/audit.ts

Lines changed: 2 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -25,12 +25,8 @@ import { matchesIgnore, RESOURCES_DIR } from "./config.ts";
2525
import { credentialReverseMap } from "./credentials.ts";
2626
import { classifyDrift } from "./drift.ts";
2727
import { findOrphanResourceIds } from "./new-file-gate.ts";
28-
import {
29-
canonicalizeForHash,
30-
fetchAllResources,
31-
listExistingResourceIds,
32-
type VapiResource,
33-
} from "./pull.ts";
28+
import { canonicalizeForHash, type VapiResource } from "./canonical.ts";
29+
import { fetchAllResources, listExistingResourceIds } from "./pull.ts";
3430
import { FOLDER_MAP, hashLocalResource } from "./resources.ts";
3531
import { extractBaseSlug, slugify } from "./slug-utils.ts";
3632
import { hashPayload, loadState } from "./state.ts";

src/canonical.ts

Lines changed: 223 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,223 @@
1+
// ─────────────────────────────────────────────────────────────────────────────
2+
// Canonical content basis for resources.
3+
//
4+
// A platform resource (UUID references, credential UUIDs, prompt inline) and a
5+
// local file (resourceId references, credential names, prompt in the md body)
6+
// are two encodings of the SAME logical content. Drift detection only works if
7+
// every site hashes them in ONE shared basis. That basis is defined here.
8+
//
9+
// This module is deliberately dependency-light (credentials + types only) so
10+
// that pull.ts, push.ts, audit.ts, AND drift.ts can all import it without the
11+
// import cycle that previously trapped this logic inside pull.ts (pull imports
12+
// drift, so drift could not import back — it grew a divergent copy instead).
13+
// ─────────────────────────────────────────────────────────────────────────────
14+
15+
import { replaceCredentialRefs } from "./credentials.ts";
16+
import type { ResourceType, StateFile } from "./types.ts";
17+
18+
export interface VapiResource {
19+
id: string;
20+
name?: string;
21+
[key: string]: unknown;
22+
}
23+
24+
// Fields to remove before hashing/writing (server-managed or computed).
25+
// Single source of truth — drift's old private `SERVER_FIELDS` was an exact
26+
// duplicate of this and drifted out of sync risk-free only by luck.
27+
export const EXCLUDED_FIELDS = [
28+
"id",
29+
"orgId",
30+
"createdAt",
31+
"updatedAt",
32+
"analyticsMetadata",
33+
"isDeleted",
34+
// Computed/derived fields that shouldn't be synced back
35+
"isServerUrlSecretSet", // Computed: indicates if server URL secret is set
36+
"workflowIds", // Server-managed: workflows are a separate resource type
37+
];
38+
39+
export function cleanResource(resource: VapiResource): Record<string, unknown> {
40+
const cleaned: Record<string, unknown> = {};
41+
42+
// Preserve `null` values: the API uses `null` to represent an intentionally
43+
// cleared field (e.g. `voicemailMessage: null`), which is semantically
44+
// different from an absent field. Stripping it on pull would cause the next
45+
// push to drop the clear and re-apply any prior value still on the server.
46+
for (const [key, value] of Object.entries(resource)) {
47+
if (!EXCLUDED_FIELDS.includes(key) && value !== undefined) {
48+
cleaned[key] = value;
49+
}
50+
}
51+
52+
return cleaned;
53+
}
54+
55+
// uuid -> resourceId, for a single resource type's state section.
56+
export function buildReverseMap(
57+
state: StateFile,
58+
resourceType: ResourceType,
59+
): Map<string, string> {
60+
const map = new Map<string, string>();
61+
const stateSection = state[resourceType];
62+
63+
for (const [resourceId, entry] of Object.entries(stateSection)) {
64+
map.set(entry.uuid, resourceId);
65+
}
66+
67+
return map;
68+
}
69+
70+
// ─────────────────────────────────────────────────────────────────────────────
71+
// Reference Resolution (UUID -> resourceId)
72+
// ─────────────────────────────────────────────────────────────────────────────
73+
74+
export function resolveReferencesToResourceIds(
75+
resource: Record<string, unknown>,
76+
state: StateFile,
77+
): Record<string, unknown> {
78+
const toolsMap = buildReverseMap(state, "tools");
79+
const assistantsMap = buildReverseMap(state, "assistants");
80+
const structuredOutputsMap = buildReverseMap(state, "structuredOutputs");
81+
const personalitiesMap = buildReverseMap(state, "personalities");
82+
const scenariosMap = buildReverseMap(state, "scenarios");
83+
const simulationsMap = buildReverseMap(state, "simulations");
84+
85+
const resolved = { ...resource };
86+
87+
// Resolve toolIds in model
88+
if (resolved.model && typeof resolved.model === "object") {
89+
const model = { ...(resolved.model as Record<string, unknown>) };
90+
if (Array.isArray(model.toolIds)) {
91+
model.toolIds = model.toolIds.map(
92+
(uuid: string) => toolsMap.get(uuid) ?? uuid,
93+
);
94+
}
95+
resolved.model = model;
96+
}
97+
98+
// Resolve structuredOutputIds in artifactPlan
99+
if (resolved.artifactPlan && typeof resolved.artifactPlan === "object") {
100+
const artifactPlan = {
101+
...(resolved.artifactPlan as Record<string, unknown>),
102+
};
103+
if (Array.isArray(artifactPlan.structuredOutputIds)) {
104+
artifactPlan.structuredOutputIds = artifactPlan.structuredOutputIds.map(
105+
(uuid: string) => structuredOutputsMap.get(uuid) ?? uuid,
106+
);
107+
}
108+
resolved.artifactPlan = artifactPlan;
109+
}
110+
111+
// Resolve assistantIds in structured outputs (API returns camelCase)
112+
if (Array.isArray(resolved.assistantIds)) {
113+
resolved.assistant_ids = (resolved.assistantIds as string[]).map(
114+
(uuid: string) => assistantsMap.get(uuid) ?? uuid,
115+
);
116+
delete resolved.assistantIds;
117+
}
118+
119+
// Resolve assistantId in tool destinations (handoff tools)
120+
if (Array.isArray(resolved.destinations)) {
121+
resolved.destinations = (
122+
resolved.destinations as Record<string, unknown>[]
123+
).map((dest) => {
124+
if (typeof dest.assistantId === "string") {
125+
return {
126+
...dest,
127+
assistantId: assistantsMap.get(dest.assistantId) ?? dest.assistantId,
128+
};
129+
}
130+
return dest;
131+
});
132+
}
133+
134+
// Resolve members[].assistantId in squads
135+
if (Array.isArray(resolved.members)) {
136+
resolved.members = (resolved.members as Record<string, unknown>[]).map(
137+
(member) => {
138+
const resolvedMember = { ...member };
139+
if (typeof member.assistantId === "string") {
140+
resolvedMember.assistantId =
141+
assistantsMap.get(member.assistantId) ?? member.assistantId;
142+
}
143+
// Resolve assistantDestinations[].assistantId
144+
if (Array.isArray(member.assistantDestinations)) {
145+
resolvedMember.assistantDestinations = (
146+
member.assistantDestinations as Record<string, unknown>[]
147+
).map((dest) => {
148+
if (typeof dest.assistantId === "string") {
149+
return {
150+
...dest,
151+
assistantId:
152+
assistantsMap.get(dest.assistantId) ?? dest.assistantId,
153+
};
154+
}
155+
return dest;
156+
});
157+
}
158+
return resolvedMember;
159+
},
160+
);
161+
}
162+
163+
// Resolve personalityId in simulations
164+
if (typeof resolved.personalityId === "string") {
165+
resolved.personalityId =
166+
personalitiesMap.get(resolved.personalityId) ?? resolved.personalityId;
167+
}
168+
169+
// Resolve scenarioId in simulations
170+
if (typeof resolved.scenarioId === "string") {
171+
resolved.scenarioId =
172+
scenariosMap.get(resolved.scenarioId) ?? resolved.scenarioId;
173+
}
174+
175+
// Resolve simulationIds in simulation suites
176+
if (Array.isArray(resolved.simulationIds)) {
177+
resolved.simulationIds = (resolved.simulationIds as string[]).map(
178+
(uuid: string) => simulationsMap.get(uuid) ?? uuid,
179+
);
180+
}
181+
182+
return resolved;
183+
}
184+
185+
// ─────────────────────────────────────────────────────────────────────────────
186+
// Canonicalization for content-hashing
187+
// ─────────────────────────────────────────────────────────────────────────────
188+
189+
/**
190+
* Single source of truth for the canonical hash basis of a platform resource.
191+
*
192+
* Encodes the full pipeline: `cleanResource → resolveReferencesToResourceIds
193+
* → replaceCredentialRefs → _platformDefault marker injection`.
194+
*
195+
* ALL hash sites MUST use this helper — pull-write fallback, the pull
196+
* classifier, the audit (audit.ts/checkContentDrift), and the push drift check
197+
* (drift.ts). Any divergence (a missing step, a different mutation order) makes
198+
* the recomputed `platformHash` disagree with the stored `lastPulledHash` from
199+
* a prior pull, producing permanent phantom `both-diverged` reports that only
200+
* `--overwrite` can clear.
201+
*
202+
* Note: this is the *pre-write* canonical form (in-memory). At write sites,
203+
* `lastPulledHash` should still be sourced from `hashLocalResource(...)` after
204+
* the file is on disk, because YAML round-trip / MD frontmatter serialization
205+
* is not guaranteed to be identity-preserving. This helper is the correct
206+
* fallback when no file write happened (bootstrap mode) or when reading a
207+
* platform payload for classifier/audit/drift purposes.
208+
*/
209+
export function canonicalizeForHash(
210+
resource: VapiResource,
211+
state: StateFile,
212+
credReverse: Map<string, string>,
213+
): Record<string, unknown> {
214+
const cleaned = cleanResource(resource);
215+
const resolved = resolveReferencesToResourceIds(cleaned, state);
216+
const withCredNames = replaceCredentialRefs(resolved, credReverse);
217+
const isPlatformDefault =
218+
resource.orgId === null || resource.orgId === undefined;
219+
if (isPlatformDefault) {
220+
withCredNames._platformDefault = true;
221+
}
222+
return withCredNames;
223+
}

0 commit comments

Comments
 (0)