diff --git a/docs/learnings/sync-behavior.md b/docs/learnings/sync-behavior.md index edaf375..bbb3eed 100644 --- a/docs/learnings/sync-behavior.md +++ b/docs/learnings/sync-behavior.md @@ -99,6 +99,35 @@ UUIDs → names): | `apply` (default `--resolve=defer`) | pull defers → push prompts for **exactly the conflicted resources**; clean ones flow silently | | `apply --resolve=ours` | no questions: pull re-baselines, push runs with `--overwrite` (CI semantics; dashboard edits lose) | +### Conflict timing context (advisory) + +When a 3-way conflict is reported, each entry carries a timing line beside the +hashes: + +``` + - assistants/intake + local-hash: 3f9a1c2b… platform-hash: 8e01d4aa… last-pulled: 3f9a1c2b… + dashboard changed 2026-08-01 15:00Z, your file 2026-08-01 12:00Z — dashboard is 3h newer +``` + +It is a hint, never a verdict, and the engine still refuses to choose. Three +reasons it cannot be trusted as one: + +- `updatedAt` is bumped by **our own pushes**, so a "newer" dashboard often just + means you pushed a few minutes ago. +- The local mtime is reset by `git clone` and `git checkout`, so on a fresh + checkout every file looks edited seconds ago. +- **Later does not mean supersedes.** If you changed the prompt and a teammate + changed the voice, both edits deserve to survive; last-write-wins would discard + one silently. + +Sub-minute gaps are reported as "within a minute of each other" rather than +picking a winner, because clock skew is the same order of magnitude as the gap. + +A previous state schema stored `lastPulledAt` for this purpose and it was +deliberately removed in favour of content hashes. This line reads `updatedAt` +from the live response and the file's mtime at report time; it persists nothing. + ### 5. Both changed identically (L = D, stale baseline) Both `pull` and `push` treat this as clean (live sides agree — nothing to diff --git a/docs/learnings/tools.md b/docs/learnings/tools.md index 9cb4dbe..ed1cb70 100644 --- a/docs/learnings/tools.md +++ b/docs/learnings/tools.md @@ -466,3 +466,18 @@ The LLM produces only `name` and `email` (what the caller spoke). The orchestrat For belt-and-braces, pair static parameters with HMAC body signing so the backend verifies sender + content, not just channel. For the trust-tier breakdown of which Liquid variables are safe to use here (`{{ customer.number }}`, `{{ call.id }}`, etc.) vs. which are LLM-derived and not, see [assistants.md → Liquid Variable Bag and Trust Tiers](assistants.md#liquid-variable-bag-and-trust-tiers). + +## Handoff/transfer tools reference assistants (dependency cycle) + +Tools are pushed before assistants, because assistants reference tools. A +handoff or transfer tool references an assistant, which inverts that for the +tool in question. The engine handles it in two passes: the tool is created (or +updated) without its unresolved assistant destinations, then a linking pass +PATCHes the real destinations once every assistant exists. + +Consequence worth knowing: on a push where the referenced assistant is not in +state, the tool's `destinations` are deliberately **not** sent. They are set by +the linking pass at the end of the same push. If you scope a push to +`--type tools` while the assistant is untracked, the destinations on the +dashboard are left as-is rather than cleared — the engine omits the key instead +of sending a partial array, because PATCH replaces whatever it receives. diff --git a/improvements.md b/improvements.md index 442ab8e..97e8a15 100644 --- a/improvements.md +++ b/improvements.md @@ -79,6 +79,7 @@ you which stack PR closes the row.** | 25 | Interactive flows lack automated coverage | Picker/conflict-prompt regressions ship silently | None | Open — scheduled for the test-update iteration | | 26 | Rollback is snapshot replay, not transaction rollback | Creates/deletes/state drift are not fully undone | #3 | Open — document/plan transactional rollback | | 27 | List endpoints read one unpaginated page at a time | >100-resource fleets got truncated orphan detection | None | RESOLVED 2026-08-01 (consumers gap open) | +| 28 | Handoff tools 400 on first push into an empty org | Push aborts before the assistant-linking pass runs | None | RESOLVED 2026-08-01 | **Active backlog after cleanup:** `#2`, `#6`, `#8`, `#12`, `#20`, `#24–#26`, and the open remainder of `#27` (wiring the listing-completeness verdict into push/delete/audit, and moving `cleanup.ts` onto the shared pager). Resolved entries stay in this file as historical incident notes per the maintenance directive; stale superseded backlog rows are not duplicated. @@ -1364,6 +1365,62 @@ so they still report confidently on a partial view. `cleanup.ts` has its own loc `vapiGet` and stays unpaginated (`src/cleanup.ts:193`); it fails safe, since a truncated listing finds *fewer* dashboard orphans to delete. +## 28. Handoff/transfer tools 400 on a first push into an empty org + +**[RESOLVED 2026-08-01]** + +**Discovered:** on a customer repo. `PATCH /tool/d787c351… → 400 Assistant with +ID "clinical-stage-1-a4598432" not found`, aborting the whole push. + +### Problem + +Tools are applied before assistants, because assistants reference tools. A +handoff/transfer tool references an *assistant*, which inverts the dependency for +that subset — a genuine cycle. The update path sent the unresolved assistant slug +to the API, which rejected it. + +### Current behavior (Verified) + +The engine already resolves the cycle in two passes. `applyTool` +(`src/push.ts`) strips unresolved assistant destinations from the **create** +payload, and `updateToolAssistantRefs` PATCHes the real destinations once every +assistant exists. The **update** payload had no equivalent: it was +`removeExcludedKeys(payload, "tools")` with the raw slug still in +`destinations[].assistantId`. Any tool that already existed on the platform while +its referenced assistant was not yet in state produced a 400, and because +`applyTool` rethrows, the push aborted before the linking pass ran. + +Reproduces whenever a tool exists remotely and its assistant does not exist +locally in state — a first push into an empty org, a re-pointed handoff, or a +`--type tools` push. + +### Risk + +A first push into a fresh org fails partway with an error that names an assistant +rather than the tool, so the cause reads as an assistant problem. Resources +applied before the failing tool stay applied, so the org is left half-configured. + +### Current mitigation + +Push assistants first (`npm run push -- --type assistants`), then push +everything. + +### Possible fix + +Implemented: `omitUnresolvedDestinations` drops the whole `destinations` key from +the update payload when any entry is unresolved, letting the existing linking pass +set the real value. + +Omitting the key matters more than filtering the array. Vapi PATCH replaces the +keys it receives, so sending a filtered array would wipe destinations that are +live on the dashboard whenever the referenced assistant is merely untracked +locally. An absent key is left alone. Covered by +`tests/tool-assistant-cycle.test.ts`. + +### Status + +**RESOLVED 2026-08-01.** + --- ## Out of scope (intentionally not improvements) diff --git a/src/pull.ts b/src/pull.ts index 85a111c..4e71fea 100644 --- a/src/pull.ts +++ b/src/pull.ts @@ -667,6 +667,93 @@ function findLocalResourcePath( ].find((p) => existsSync(p)); } +// ───────────────────────────────────────────────────────────────────────────── +// Conflict timing context +// +// A 3-way conflict report used to show three 8-character hash prefixes, which +// tell a human nothing about which side to keep. Timestamps do — but only as a +// hint, never as the decision: +// +// - `updatedAt` is bumped by OUR OWN pushes, so a "newer" dashboard often just +// means you pushed a few minutes ago, not that a teammate changed anything. +// - the local mtime is reset by `git clone` and `git checkout`, so on a fresh +// checkout every file looks like it was edited seconds ago. +// - "later" does not mean "supersedes". Two edits to different fields both +// deserve to survive, and last-write-wins would silently discard one. +// +// So this is printed to help a human pick a `--resolve` mode. The engine still +// refuses to choose. A previous schema stored `lastPulledAt` for this and it was +// deliberately dropped in favour of content hashes; nothing here brings it back. +// ───────────────────────────────────────────────────────────────────────────── + +function formatAge(ms: number): string { + const mins = Math.round(ms / 60_000); + if (mins < 60) return `${mins}m`; + const hours = Math.round(mins / 60); + if (hours < 48) return `${hours}h`; + return `${Math.round(hours / 24)}d`; +} + +function isoMinute(date: Date): string { + return `${date.toISOString().slice(0, 16).replace("T", " ")}Z`; +} + +/** + * Human-readable timing for one conflicted resource, or `undefined` when + * neither side offers a usable timestamp. Exported for tests. + */ +export function conflictTimingHint(options: { + dashboardUpdatedAt?: unknown; + localModifiedMs?: number; +}): string | undefined { + const remote = + typeof options.dashboardUpdatedAt === "string" + ? new Date(options.dashboardUpdatedAt) + : undefined; + const remoteOk = remote && !Number.isNaN(remote.getTime()); + const local = + typeof options.localModifiedMs === "number" && + Number.isFinite(options.localModifiedMs) + ? new Date(options.localModifiedMs) + : undefined; + + if (!remoteOk && !local) return undefined; + if (remoteOk && !local) return `dashboard changed ${isoMinute(remote)}`; + if (!remoteOk && local) return `your file changed ${isoMinute(local)}`; + + const delta = remote!.getTime() - local!.getTime(); + const which = + Math.abs(delta) < 60_000 + ? "within a minute of each other" + : delta > 0 + ? `dashboard is ${formatAge(delta)} newer` + : `your file is ${formatAge(-delta)} newer`; + return `dashboard changed ${isoMinute(remote!)}, your file ${isoMinute(local!)} — ${which}`; +} + +// Reads the local file's mtime for the timing hint. Returns undefined rather +// than throwing: a missing file or an unreadable stat must never break the +// conflict report. +function localModifiedMs( + resourceType: ResourceType, + resourceId: string, +): number | undefined { + try { + const path = findLocalResourcePath(FOLDER_MAP[resourceType], resourceId); + return path ? statSync(path).mtimeMs : undefined; + } catch { + return undefined; + } +} + +function timingLine(entry: BothDivergedResource): string { + const hint = conflictTimingHint({ + dashboardUpdatedAt: entry.resource.updatedAt, + localModifiedMs: localModifiedMs(entry.resourceType, entry.resourceId), + }); + return hint ? `\n ${hint}` : ""; +} + export interface PullOptions { force?: boolean; bootstrap?: boolean; @@ -1057,7 +1144,8 @@ async function resolveBothDivergedResources(options: { ); for (const entry of bothDiverged) { console.log( - ` - ${FOLDER_MAP[entry.resourceType]}/${entry.resourceId}`, + ` - ${FOLDER_MAP[entry.resourceType]}/${entry.resourceId}` + + timingLine(entry), ); } return { exitCode: 0 }; @@ -1070,7 +1158,8 @@ async function resolveBothDivergedResources(options: { for (const entry of bothDiverged) { console.error( ` - ${entry.resourceType}/${entry.resourceId}\n` + - ` local-hash: ${entry.localHash.slice(0, 8)}… platform-hash: ${entry.platformHash.slice(0, 8)}… last-pulled: ${entry.lastPulledHash.slice(0, 8)}…`, + ` local-hash: ${entry.localHash.slice(0, 8)}… platform-hash: ${entry.platformHash.slice(0, 8)}… last-pulled: ${entry.lastPulledHash.slice(0, 8)}…` + + timingLine(entry), ); } return { exitCode: 1 }; @@ -1084,7 +1173,8 @@ async function resolveBothDivergedResources(options: { for (const entry of bothDiverged) { console.error( ` - ${FOLDER_MAP[entry.resourceType]}/${entry.resourceId}\n` + - ` local-hash: ${entry.localHash.slice(0, 8)}… platform-hash: ${entry.platformHash.slice(0, 8)}… last-pulled: ${entry.lastPulledHash.slice(0, 8)}…`, + ` local-hash: ${entry.localHash.slice(0, 8)}… platform-hash: ${entry.platformHash.slice(0, 8)}… last-pulled: ${entry.lastPulledHash.slice(0, 8)}…` + + timingLine(entry), ); } console.error( @@ -1096,6 +1186,15 @@ async function resolveBothDivergedResources(options: { console.error( " --resolve=fail exit non-zero without writing anything (CI mode — fail the build so a human investigates)", ); + console.error( + "\n Timestamps above are a hint, not a verdict: your own pushes bump the dashboard's", + ); + console.error( + " updatedAt, and git clone/checkout resets local file times. Newer does not mean correct —", + ); + console.error( + " two edits to different fields both deserve to survive.", + ); return { exitCode: 1 }; } diff --git a/src/push.ts b/src/push.ts index c25223e..002727d 100644 --- a/src/push.ts +++ b/src/push.ts @@ -604,12 +604,30 @@ export async function applyTool( stateSection: state.tools, fullState: state, updateEndpoint: `/tool/${existingUuid}`, - updatePayload: removeExcludedKeys(payload, "tools"), + updatePayload: omitUnresolvedDestinations( + removeExcludedKeys(payload, "tools"), + data as Record, + ), createEndpoint: "/tool", createPayload: payloadForCreate, }); } +// A destination is unresolved when reference resolution left the assistantId +// exactly as the file wrote it — i.e. the slug is not in state yet, so no UUID +// could be substituted. +function isUnresolvedDestination( + resolvedDest: Record | undefined, + originalDest: Record | undefined, +): boolean { + if (!resolvedDest || typeof resolvedDest.assistantId !== "string") + return false; + if (!originalDest || typeof originalDest.assistantId !== "string") + return false; + const originalId = (originalDest.assistantId as string).split("##")[0]?.trim(); + return resolvedDest.assistantId === originalId; +} + // Strip destinations with unresolved assistantIds (where original equals resolved = not found in state) function stripUnresolvedAssistantDestinations( resolved: Record, @@ -622,19 +640,47 @@ function stripUnresolvedAssistantDestinations( const originalDests = original.destinations as Record[]; const resolvedDests = resolved.destinations as Record[]; - // Filter out destinations where assistantId wasn't resolved (still matches original) - const filteredDests = resolvedDests.filter((dest, idx) => { - if (typeof dest.assistantId !== "string") return true; - const origDest = originalDests[idx]; - if (!origDest || typeof origDest.assistantId !== "string") return true; - // Keep if resolved (UUID format) or no original assistantId - const originalId = (origDest.assistantId as string).split("##")[0]?.trim(); - return dest.assistantId !== originalId; - }); + const filteredDests = resolvedDests.filter( + (dest, idx) => !isUnresolvedDestination(dest, originalDests[idx]), + ); return { ...resolved, destinations: filteredDests }; } +// The update-path counterpart, and the reason a first push into an empty org +// used to fail with `400 Assistant with ID "" not found`. +// +// Tools are applied before assistants (tools are a dependency of assistants), +// but a handoff/transfer tool references an assistant — a genuine cycle. On a +// CREATE the unresolved destinations are stripped and `updateToolAssistantRefs` +// links them once every assistant exists. The UPDATE path had no equivalent, so +// it sent the raw slug, the API rejected it, and the push aborted before the +// linking pass could run. +// +// This omits the whole `destinations` key rather than sending a filtered array: +// PATCH replaces the keys it receives, so a filtered array would wipe +// destinations that are live on the dashboard whenever the local assistant is +// merely untracked (a `--type tools` push, for instance). Omitting the key +// leaves the platform value untouched, and the linking pass sets the real value. +export function omitUnresolvedDestinations( + payload: Record, + original: Record, +): Record { + if (!Array.isArray(payload.destinations)) return payload; + + const originalDests = Array.isArray(original.destinations) + ? (original.destinations as Record[]) + : []; + const anyUnresolved = ( + payload.destinations as Record[] + ).some((dest, idx) => isUnresolvedDestination(dest, originalDests[idx])); + + if (!anyUnresolved) return payload; + + const { destinations: _omitted, ...rest } = payload; + return rest; +} + export async function applyStructuredOutput( resource: ResourceFile, state: StateFile, diff --git a/tests/conflict-timing.test.ts b/tests/conflict-timing.test.ts new file mode 100644 index 0000000..9771af4 --- /dev/null +++ b/tests/conflict-timing.test.ts @@ -0,0 +1,92 @@ +import assert from "node:assert/strict"; +import test from "node:test"; + +// ───────────────────────────────────────────────────────────────────────────── +// A 3-way conflict report used to show three 8-character hash prefixes and +// nothing else, which tells a human nothing about which side to keep. These +// tests pin the advisory timing line that replaced that gap. +// +// "Advisory" is the whole contract. The engine must keep refusing to choose, +// because neither timestamp is authoritative: `updatedAt` is bumped by our own +// pushes, local mtime is reset by clone and checkout, and two edits to different +// fields both deserve to survive regardless of which landed last. +// ───────────────────────────────────────────────────────────────────────────── + +process.argv = ["node", "test", "test-fixture-org"]; +process.env.VAPI_TOKEN = process.env.VAPI_TOKEN || "test-token-not-used"; + +const { conflictTimingHint } = await import("../src/pull.ts"); + +const T0 = Date.parse("2026-08-01T12:00:00.000Z"); + +test("reports both sides and which is newer when the dashboard moved later", () => { + const hint = conflictTimingHint({ + dashboardUpdatedAt: new Date(T0 + 3 * 3_600_000).toISOString(), + localModifiedMs: T0, + }); + + assert.match(hint as string, /dashboard changed 2026-08-01 15:00Z/); + assert.match(hint as string, /your file 2026-08-01 12:00Z/); + assert.match(hint as string, /dashboard is 3h newer/); +}); + +test("reports the local file as newer when it moved later", () => { + const hint = conflictTimingHint({ + dashboardUpdatedAt: new Date(T0).toISOString(), + localModifiedMs: T0 + 2 * 86_400_000, + }); + + assert.match(hint as string, /your file is 2d newer/); +}); + +test("edits within a minute are not called a winner", () => { + // Sub-minute ordering is noise: clock skew between your machine and the + // platform is the same order of magnitude as the gap. + const hint = conflictTimingHint({ + dashboardUpdatedAt: new Date(T0 + 20_000).toISOString(), + localModifiedMs: T0, + }); + + assert.match(hint as string, /within a minute of each other/); + assert.doesNotMatch(hint as string, /newer/); +}); + +test("minutes are used below an hour", () => { + const hint = conflictTimingHint({ + dashboardUpdatedAt: new Date(T0 + 25 * 60_000).toISOString(), + localModifiedMs: T0, + }); + + assert.match(hint as string, /dashboard is 25m newer/); +}); + +test("one side alone still produces a usable line", () => { + // A resource with no local file, or an unreadable stat, must not blank the + // whole report. + assert.match( + conflictTimingHint({ dashboardUpdatedAt: new Date(T0).toISOString() }) as string, + /^dashboard changed 2026-08-01 12:00Z$/, + ); + assert.match( + conflictTimingHint({ localModifiedMs: T0 }) as string, + /^your file changed 2026-08-01 12:00Z$/, + ); +}); + +test("no usable timestamp yields no line at all", () => { + // The caller appends this to an existing bullet, so an empty hint has to be + // distinguishable from a hint that happens to be short. + assert.equal(conflictTimingHint({}), undefined); + assert.equal(conflictTimingHint({ dashboardUpdatedAt: 1754049600000 }), undefined); + assert.equal(conflictTimingHint({ dashboardUpdatedAt: "not a date" }), undefined); + assert.equal(conflictTimingHint({ localModifiedMs: Number.NaN }), undefined); +}); + +test("a malformed dashboard timestamp falls back to the local side", () => { + const hint = conflictTimingHint({ + dashboardUpdatedAt: "2026-13-45T99:99:99Z", + localModifiedMs: T0, + }); + + assert.match(hint as string, /^your file changed 2026-08-01 12:00Z$/); +}); diff --git a/tests/tool-assistant-cycle.test.ts b/tests/tool-assistant-cycle.test.ts new file mode 100644 index 0000000..7cb4f14 --- /dev/null +++ b/tests/tool-assistant-cycle.test.ts @@ -0,0 +1,116 @@ +import assert from "node:assert/strict"; +import test from "node:test"; + +// ───────────────────────────────────────────────────────────────────────────── +// Tools are applied before assistants because assistants reference tools. But a +// handoff/transfer tool references an assistant, so that subset inverts the +// dependency — a genuine cycle. +// +// The engine already resolves it in two passes: the CREATE path strips +// unresolved assistant destinations, and `updateToolAssistantRefs` links them +// once every assistant exists. The UPDATE path had no equivalent, so a first +// push into an empty org sent the raw slug and the API answered: +// +// PATCH /tool/ → 400 Assistant with ID "clinical-stage-1-a4598432" not found +// +// which aborted the push before the linking pass could run. +// ───────────────────────────────────────────────────────────────────────────── + +process.argv = ["node", "test", "test-fixture-org"]; +process.env.VAPI_TOKEN = process.env.VAPI_TOKEN || "test-token-not-used"; + +const { omitUnresolvedDestinations } = await import("../src/push.ts"); + +const UUID = "8f14e45f-ceea-467a-9f1b-1a1b2c3d4e5f"; + +test("an unresolved assistant destination drops the whole destinations key", async () => { + // Unresolved = resolution left the slug exactly as written, because the + // assistant is not in state yet. + const original = { + type: "transferCall", + destinations: [{ type: "assistant", assistantId: "clinical-stage-1" }], + }; + const payload = { + type: "transferCall", + destinations: [{ type: "assistant", assistantId: "clinical-stage-1" }], + }; + + const result = omitUnresolvedDestinations(payload, original); + + assert.ok( + !("destinations" in result), + "the key must be absent so PATCH leaves the platform value untouched", + ); + assert.equal(result.type, "transferCall", "the rest of the payload survives"); +}); + +test("a resolved destination is sent unchanged", async () => { + const original = { + destinations: [{ type: "assistant", assistantId: "clinical-stage-1" }], + }; + const payload = { + destinations: [{ type: "assistant", assistantId: UUID }], + }; + + const result = omitUnresolvedDestinations(payload, original); + + assert.deepEqual(result.destinations, [ + { type: "assistant", assistantId: UUID }, + ]); +}); + +test("a trailing YAML comment on the reference still counts as resolved", async () => { + // `assistantId: clinical-stage-1 ## human note` — the comment is part of the + // authored string, so the comparison has to strip it or every reference would + // look unresolved. + const original = { + destinations: [ + { type: "assistant", assistantId: "clinical-stage-1 ## stage one" }, + ], + }; + const payload = { destinations: [{ type: "assistant", assistantId: UUID }] }; + + const result = omitUnresolvedDestinations(payload, original); + + assert.ok("destinations" in result, "a resolved reference is still sent"); +}); + +test("one unresolved destination among several omits the key, not just that entry", async () => { + // Sending a filtered array would PATCH-replace `destinations` and drop the + // resolved sibling from the dashboard. The linking pass rewrites the whole + // array afterwards, so omitting is both safe and complete. + const original = { + destinations: [ + { type: "assistant", assistantId: "stage-one" }, + { type: "assistant", assistantId: "stage-two" }, + ], + }; + const payload = { + destinations: [ + { type: "assistant", assistantId: UUID }, + { type: "assistant", assistantId: "stage-two" }, + ], + }; + + const result = omitUnresolvedDestinations(payload, original); + + assert.ok( + !("destinations" in result), + "a partially resolved array must not be sent", + ); +}); + +test("a tool with no destinations is untouched", async () => { + const payload = { type: "function", function: { name: "lookup" } }; + const result = omitUnresolvedDestinations(payload, payload); + assert.deepEqual(result, payload); +}); + +test("non-assistant destinations never block the update", async () => { + // Number/SIP destinations carry no assistantId, so they are always sendable. + const payload = { + destinations: [{ type: "number", number: "+15550000000" }], + }; + const result = omitUnresolvedDestinations(payload, payload); + assert.ok("destinations" in result); +});