diff --git a/docs/learnings/structured-outputs.md b/docs/learnings/structured-outputs.md index 9621761..7685a9a 100644 --- a/docs/learnings/structured-outputs.md +++ b/docs/learnings/structured-outputs.md @@ -97,3 +97,47 @@ Structured outputs are the primary way to measure voice agent performance. Commo **Recommendation:** For squad-wide KPIs that must populate on **every** ending (VM-only, classifier-only, and live-agent), use **`squad.membersOverrides.analysisPlan.structuredDataPlan`** plus **`membersOverrides.artifactPlan.fullMessageHistoryEnabled: true`**. Remove per-assistant duplicate plans. Keep standalone structured outputs for evals, dashboard analytics, or schemas you want versioned as separate resources. Full YAML pattern and merge-order notes: [squads.md → Squad-level post-call extraction via `membersOverrides`](squads.md#squad-level-post-call-extraction-via-membersoverrides-multi-member-squads). + +--- + +## assistant_ids and the update/linking cycle (dependency cycle) + +Structured outputs are pushed before assistants, same as tools — see +[tools.md → "Handoff/transfer tools reference assistants (dependency +cycle)"](tools.md#handofftransfer-tools-reference-assistants-dependency-cycle) +for the general shape. A structured output's `assistant_ids` references an +assistant, which inverts the push order for that field. The engine handles it +the same two-pass way: on create, `assistantIds` is stripped entirely (no +assistants exist yet); on update, it's resolved to whatever already exists in +state. Either way, a linking pass then sets the real value once every +assistant in the push has been applied. + +The difference from tool destinations is what happens when a reference still +doesn't resolve. `assistantIds` is a flat array with no shape to signal "leave +this one for later" — so instead of sending a shorter array, the engine omits +the whole `assistantIds` key from the update PATCH whenever any authored +`assistant_ids` entry fails to resolve. Sending a filtered array would PATCH- +replace the platform's current list, silently unlinking every live-but- +untracked assistant the array left out — the update never shrinks the +dashboard's array as a side effect of an unrelated edit. + +The linking pass (which runs once every assistant in the push has been +applied) can still find an unresolved reference — genuinely absent from both +state and the local repo, not just not-yet-applied. When that happens it +**skips that structured output** rather than PATCHing the partial list, and +logs a warning naming the unresolved reference(s): + +``` +⚠️ Structured output "" still references unresolved assistant(s): . Leaving this structured output's assistant links untouched on the platform — they will link on a future push once those assistants exist. +``` + +**One asymmetry worth knowing versus tool destinations:** for structured +outputs, an **untracked raw UUID counts as unresolved**, not as a legitimate +platform-only reference. Tool destinations let a raw UUID through even when +it's untracked in state (it might be a real dashboard assistant the repo +just doesn't manage). Structured outputs can't take that risk — there's no +later repair pass for a structured output's assistant links the way there is +for tool destinations, so if the linking pass got it wrong here, the wipe +would be permanent until someone noticed and re-pushed by hand. Treating an +untracked UUID as unresolved means it skips-and-warns instead of silently +shipping a wipe. 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..592fdec 100644 --- a/docs/learnings/tools.md +++ b/docs/learnings/tools.md @@ -466,3 +466,34 @@ 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 by the main +create/update. They are set by the linking pass at the end of the same push, +once every assistant in the push has been applied. 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. + +The linking pass itself can still find an unresolved destination — not +"not yet applied this push" but genuinely absent from both state and the +local repo. In that case it does **not** send the raw slug. It **skips that +tool** and logs a warning naming the unresolved reference(s): + +``` +⚠️ Tool "" still references unresolved assistant destination(s): . Leaving this tool's destinations untouched on the platform — they will link on a future push once those assistants exist. +``` + +The rest of the push continues normally, and the destinations link +automatically on a later push once the assistant exists. Previously the +linking pass sent the raw slug straight through, and the API's `400 +Assistant not found` aborted the whole push at the very end — after every +other resource had already applied. diff --git a/improvements.md b/improvements.md index 442ab8e..1e8fa53 100644 --- a/improvements.md +++ b/improvements.md @@ -79,8 +79,12 @@ 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 | +| 29 | SO linking sent filtered `assistantIds` arrays | Silent unlink of live-but-untracked assistants | None | RESOLVED 2026-08-03 (#51) | +| 30 | Tool-linking pass could PATCH a raw assistant slug | Mid-push 400 naming the wrong resource | None | RESOLVED 2026-08-03 (#51) | +| 31 | Unresolved references handled 3 inconsistent ways, no dangling-ref check | Same authoring mistake, three different failure modes | None | Open | -**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. +**Active backlog after cleanup:** `#2`, `#6`, `#8`, `#12`, `#20`, `#24–#26`, `#31`, 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 +1368,282 @@ 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.** + +--- + +## 29. Structured-output assistant-link update/linking sent filtered `assistantIds` arrays — silent unlink of live-but-untracked assistants + +**[RESOLVED 2026-08-03] (#51)** + +**Discovered:** during the same audit that produced #30 — the tool-destination +linking pass and the structured-output linking pass share the exact same +circular-dependency shape, but the structured-output side had a materially +worse version of the gap: `assistantIds` is a flat array with no per-entry +"leave this one alone" marker, so the pre-fix code had no way to skip just the +unresolved entry. + +### Problem + +`applyStructuredOutput`'s update path and `updateStructuredOutputAssistantRefs`'s +linking pass both resolved `assistant_ids` with `resolveAssistantIds` +(`src/resolver.ts:103-110`), which silently drops any entry that fails to +resolve, then PATCHed the resulting array onto `assistantIds` regardless of +whether it was shorter than what was authored. Vapi PATCH replaces the key it +receives rather than merging, so a filtered array didn't just fail to add the +unresolved assistant — on a resource that already existed, it removed that +assistant's link on the dashboard if one was already there, with no warning +that anything had changed. + +### Current behavior (Verified) + +- `omitUnresolvedAssistantIds` (`src/push.ts:759-773`) compares the resolved + `assistantIds` length against `countAuthoredAssistantRefs` + (`src/push.ts:745-750`, counts non-empty authored `assistant_ids` entries) + and omits the `assistantIds` key from the update payload entirely when the + resolved array is shorter. Wired into `applyStructuredOutput` at + `src/push.ts:795-798`. +- `updateStructuredOutputAssistantRefs` (`src/push.ts:1038-1095`) applies the + same length check at `src/push.ts:1068-1083`: when `resolveAssistantIds` + returns fewer entries than the cleaned, non-empty authored refs, it skips + the PATCH for that structured output and logs a warning naming the + unresolved reference(s), via `assistantRefIsTracked` (`src/push.ts:1031-1036`): + + ``` + ⚠️ Structured output "" still references unresolved assistant(s): . Leaving this structured output's assistant links untouched on the platform — they will link on a future push once those assistants exist. + ``` + +- Unlike tool destinations, an **untracked raw UUID counts as unresolved** + here: `resolveAssistantId` (`src/resolver.ts:79-101`) warns and returns + `null` for an untracked UUID rather than passing it through. There is no + later pass that repairs a structured output's assistant links the way the + tool-linking pass repairs destinations, so the stricter rule is + intentional. Covered by `tests/so-assistant-omit.test.ts`. + +### Risk + +Silent — no error, no push-time signal. A structured output's live +`assistant_ids` could shrink on any push where one authored reference was +untracked, whether that reference was a typo, an assistant not yet pulled +locally, or one that was deleted from the repo. The only symptom is a KPI or +eval that quietly stops running against calls it used to cover. + +### Current mitigation + +Prior to the fix: audit each structured output's live `assistant_ids` against +the dashboard after any push where an assistant reference changed, and keep +`assistant_ids` and state in lockstep (see #11). + +### Possible fix + +Implemented: the omit-on-update / skip-and-warn-on-link guards above. + +### Status + +**RESOLVED 2026-08-03 (#51).** + +--- + +## 30. Tool-destination linking pass could still PATCH a raw assistant slug when the assistant was genuinely absent + +**[RESOLVED 2026-08-03] (#51)** + +**Discovered:** while auditing #28's fix — that entry closed the gap in the +tool's own create/update PATCH, but the separate linking pass +(`updateToolAssistantRefs`), which runs after every assistant in the push has +been applied, had the identical gap for the same reason. + +### Problem + +`updateToolAssistantRefs` (`src/push.ts:973-1021`) resolves a tool's +`destinations` again once all assistants in the push have been applied, then +PATCHed `{ destinations: resolved.destinations }` unconditionally. If a +destination's `assistantId` is genuinely absent — not in state and not in the +local repo, as opposed to merely not-yet-applied earlier in the same push — +`resolveReferences` leaves the raw slug in place (`src/resolver.ts:244-256`: +`if (resolvedId) { destination.assistantId = resolvedId }`, no `else`). The +linking pass sent that raw slug straight to the API. + +### Current behavior (Verified) + +`unresolvedDestinationSlugs` (`src/push.ts:695-714`) scans +`resolved.destinations` for any `assistantId` that still isn't a UUID after +resolution, and `updateToolAssistantRefs` (`src/push.ts:1004-1010`) skips that +tool's PATCH entirely when any are found, logging: + +``` +⚠️ Tool "" still references unresolved assistant destination(s): . Leaving this tool's destinations untouched on the platform — they will link on a future push once those assistants exist. +``` + +A destination that resolves to an untracked-but-valid UUID still flows +through unchanged (untracked UUIDs may legitimately exist on the platform; +only a still-a-slug value counts as unresolved — see #29 for why structured +outputs made the opposite call). Covered by +`tests/tool-assistant-cycle.test.ts`. + +### Risk + +`PATCH /tool/{uuid}` returned `400 Assistant with ID "" not found`, and +because the linking pass runs as the last stage of `push`, the failure landed +after every other resource in the push had already applied — the error named +an assistant, not the tool, making the actual cause non-obvious. + +### Current mitigation + +Prior to the fix: push assistants first +(`npm run push -- --type assistants`) so no destination is ever +genuinely unresolved by the time the linking pass runs. + +### Possible fix + +Implemented: the skip-and-warn guard above. No further action needed — the +destinations link automatically once the referenced assistant is added to +the repo and a subsequent push runs. + +### Status + +**RESOLVED 2026-08-03 (#51).** + +--- + +## 31. Unresolved references are handled three different ways depending on the field, and `validate.ts` has no dangling-reference check + +**Discovered:** while fixing #29 and #30 — those two entries close the +loudest and quietest failure modes for their specific fields, but the +underlying question ("what happens when a reference resolves to nothing") +still has a different answer per field, and nothing validates references +before push time. + +### Problem + +Whether an unresolved reference is dropped, deferred, or 400s depends +entirely on which field it's in. There's no single engine-wide rule, and +`validate.ts` never checks that a referenced id resolves to anything before +push. + +### Current behavior (Verified) + +Three distinct behaviors exist today, none of them a validation error: + +- **Silently filtered (array shrinks, no warning about the shrink itself):** + `model.toolIds` / root `toolIds` (`resolveToolIds`, `src/resolver.ts:46-50`), + `artifactPlan.structuredOutputIds` (`resolveStructuredOutputIds`, + `src/resolver.ts:52-77`), and structured-output `assistant_ids` → + `assistantIds` (`resolveAssistantIds`, `src/resolver.ts:103-110`) at the + point of first resolution in `resolveReferences` + (`src/resolver.ts:184-219`). Each resolver logs a per-item `⚠️` warning but + still returns a shorter array; callers other than the two guarded in + #29/#30 (e.g. `applyAssistant`'s `model.toolIds` / + `artifactPlan.structuredOutputIds`) send that shorter array straight + through with no further check. +- **Left raw → mid-push 400:** `hooks[].do[].toolId` + (`src/resolver.ts:227-241`), squad `members[].assistantId` / + `assistantDestinations[].assistantId` (`src/resolver.ts:258-282`), + `personalityId` (`src/resolver.ts:284-290`), and `scenarioId` + (`src/resolver.ts:292-298`) all follow the pattern + `if (resolvedId) { field = resolvedId }` with no `else` — an unresolved + reference keeps its original slug and is sent to the API as-is, which 400s + on whichever endpoint receives it. +- **Deferred to a linking pass:** tool `destinations[].assistantId` and + structured-output `assistant_ids`, per #29 and #30. + +`validate.ts` has no check that a referenced id resolves to something. Its +existing reference-shaped checks stop short of it: `checkLockstep`'s forward +pass explicitly steps around a missing reference — +`if (!assistant) continue; // missing-reference is a different class` +(`src/validate.ts:120`) — because the SO↔assistant lockstep check (#11) only +validates that both sides of an edge agree, not that either side exists. +`checkResourceRefs` (`src/validate.ts:456-483`, Check 6) already walks every +reference field via `extractReferencedIds` (`src/resolver.ts:351-457`), but +only to flag a reference matching `.vapi-ignore` — it doesn't flag a +reference that resolves to nothing at all. + +### Risk + +The same authoring mistake — a typo'd resource id, a reference to a file that +was deleted, a reference to a resource in the wrong org — surfaces three +different ways depending on which field it's in: a silently smaller array, a +mid-push 400 naming the wrong resource, or a skipped resource with a warning. +Fixing one field's dangling reference gives no reason to expect a different +field behaves completely differently. + +### Current mitigation + +Read `docs/learnings/tools.md` and `docs/learnings/structured-outputs.md` for +the two guarded fields; for everything else, treat any reference to a +resource that doesn't yet exist as unsafe to push until the referenced +resource exists in state. + +### Possible fix + +Extend `checkResourceRefs` (`src/validate.ts:456-483`) — it already walks +every reference field via `extractReferencedIds` for the ignored-reference +check — to also flag any id that is not a UUID, is not present in the +corresponding state section, and has no matching local resource file, as a +blocking validation finding. The ignored-reference check and a +dangling-reference check would share the same walk, just different match +conditions; it doesn't require picking one runtime behavior (filter vs. +defer vs. 400) for every field, since it stops the push before any of those +three behaviors gets a chance to run. + +### Status + +**Open.** + --- ## 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..3270e9e 100644 --- a/src/push.ts +++ b/src/push.ts @@ -604,12 +604,38 @@ 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. Both sides are cleaned of a trailing `## comment` +// before comparing: an unresolved reference authored with a comment comes +// back from `resolveReferences` with the comment still attached (resolution +// failed, so the raw string is untouched), while the original is compared +// clean — leaving the resolved side uncleaned made every commented-but- +// unresolved reference compare unequal and misclassify as resolved. +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 resolvedId = + (resolvedDest.assistantId as string).split("##")[0]?.trim() ?? ""; + const originalId = + (originalDest.assistantId as string).split("##")[0]?.trim() ?? ""; + return resolvedId === originalId; +} + // Strip destinations with unresolved assistantIds (where original equals resolved = not found in state) function stripUnresolvedAssistantDestinations( resolved: Record, @@ -622,19 +648,130 @@ 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; +} + +// Destinations that survived resolution still carrying a slug — the +// referenced assistant is in neither state nor the local repo. Cleaned of +// any trailing `## comment` the author left on the reference. +export function unresolvedDestinationSlugs(destinations: unknown): string[] { + if (!Array.isArray(destinations)) return []; + + const slugs: string[] = []; + for (const dest of destinations as unknown[]) { + if ( + !dest || + typeof dest !== "object" || + typeof (dest as Record).assistantId !== "string" + ) { + continue; + } + const cleaned = (dest as Record).assistantId as string; + const slug = cleaned.split("##")[0]?.trim() ?? ""; + if (!UUID_REGEX.test(slug)) { + slugs.push(slug); + } + } + return slugs; +} + +// Strip any trailing `## comment` from every destination's `assistantId` +// before it goes out in a PATCH body. An untracked raw UUID authored with a +// comment (`8f14…4e5f ## billing agent (unmanaged)`) fails resolution — it's +// left exactly as authored — and passes `unresolvedDestinationSlugs` (which +// cleans before checking the UUID shape), so without this the linking pass +// would PATCH the comment straight to the API and 400. A no-op for entries +// that already resolved to a bare UUID or never had a comment. +export function cleanDestinationAssistantIds(destinations: unknown): unknown { + if (!Array.isArray(destinations)) return destinations; + + return destinations.map((dest) => { + if ( + !dest || + typeof dest !== "object" || + typeof (dest as Record).assistantId !== "string" + ) { + return dest; + } + const assistantId = + ((dest as Record).assistantId as string) + .split("##")[0] + ?.trim() ?? ""; + return { ...(dest as Record), assistantId }; + }); +} + +// Count of authored `assistant_ids` entries — strings, cleaned of any +// trailing `## comment`, non-empty after trim. Shared counting rule between +// `omitUnresolvedAssistantIds` and `updateStructuredOutputAssistantRefs`. +function countAuthoredAssistantRefs(assistantIds: unknown): number { + if (!Array.isArray(assistantIds)) return 0; + return assistantIds.filter( + (ref) => typeof ref === "string" && (ref.split("##")[0]?.trim() ?? "") !== "", + ).length; +} + +// Same omit-not-filter rationale as `omitUnresolvedDestinations`: PATCH +// replaces the keys it receives, so a partially-resolved `assistantIds` +// array would wipe assistant links that are live on the dashboard whenever a +// referenced assistant is merely untracked locally (a `--type structuredOutputs` +// push, for instance). Omitting the key leaves the platform value untouched; +// `updateStructuredOutputAssistantRefs` sets the real value once every +// assistant exists. +export function omitUnresolvedAssistantIds( + payload: Record, + original: Record, +): Record { + if (!Array.isArray(original.assistant_ids)) return payload; + if (!Array.isArray(payload.assistantIds)) return payload; + + const authoredCount = countAuthoredAssistantRefs(original.assistant_ids); + if ((payload.assistantIds as unknown[]).length >= authoredCount) { + return payload; + } + + const { assistantIds: _omitted, ...rest } = payload; + return rest; +} + export async function applyStructuredOutput( resource: ResourceFile, state: StateFile, @@ -655,7 +792,10 @@ export async function applyStructuredOutput( stateSection: state.structuredOutputs, fullState: state, updateEndpoint: `/structured-output/${existingUuid}?schemaOverride=true`, - updatePayload: removeExcludedKeys(payload, "structuredOutputs"), + updatePayload: omitUnresolvedAssistantIds( + removeExcludedKeys(payload, "structuredOutputs"), + data as Record, + ), createEndpoint: "/structured-output", createPayload: payloadWithoutAssistants, }); @@ -855,9 +995,23 @@ export async function updateToolAssistantRefs( // Resolve destinations now that all assistants exist const resolved = resolveReferences(rawData, state); + // A destination can still carry a slug here if the referenced assistant + // is genuinely absent (not in state, not in the local repo) rather than + // merely not-yet-applied. Sending that slug in the PATCH would 400 the + // whole push at the very end, after every other resource already + // applied — skip this tool and let a future push link it once the + // assistant exists. + const unresolvedSlugs = unresolvedDestinationSlugs(resolved.destinations); + if (unresolvedSlugs.length > 0) { + console.warn( + ` ⚠️ Tool "${resourceId}" still references unresolved assistant destination(s): ${unresolvedSlugs.join(", ")}. Leaving this tool's destinations untouched on the platform — they will link on a future push once those assistants exist.`, + ); + continue; + } + console.log(` 🔗 Linking tool ${resourceId} to assistant destinations`); const result = await vapiRequest("PATCH", `/tool/${uuid}`, { - destinations: resolved.destinations, + destinations: cleanDestinationAssistantIds(resolved.destinations), }); // This PATCH mutates the platform AFTER the main upsert wrote its // baseline — refresh it from the linking response, or the next push would @@ -870,6 +1024,17 @@ export async function updateToolAssistantRefs( // Post-Apply: Update Structured Outputs with Assistant References // ───────────────────────────────────────────────────────────────────────────── +// Local mirror of the untracked-UUID / missing-slug check `resolveAssistantId` +// (src/resolver.ts) applies, used only to name which authored refs failed to +// resolve in the warning below — the skip decision itself is the plain length +// comparison against `countAuthoredAssistantRefs`. +function assistantRefIsTracked(ref: string, state: StateFile): boolean { + if (UUID_REGEX.test(ref)) { + return Object.values(state.assistants).some((entry) => entry.uuid === ref); + } + return !!state.assistants[ref]?.uuid; +} + export async function updateStructuredOutputAssistantRefs( structuredOutputs: ResourceFile[], state: StateFile, @@ -889,12 +1054,34 @@ export async function updateStructuredOutputAssistantRefs( const uuid = state.structuredOutputs[resourceId]?.uuid; if (!uuid) continue; + const authoredRefs = (rawData.assistant_ids as unknown[]) + .filter((ref): ref is string => typeof ref === "string") + .map((ref) => ref.split("##")[0]?.trim() ?? "") + .filter((ref) => ref !== ""); + // Resolve assistant IDs now that all assistants exist const assistantIds = resolveAssistantIds( rawData.assistant_ids as string[], state, ); + if (assistantIds.length < authoredRefs.length) { + // A referenced assistant is genuinely absent (not in state, not in the + // local repo) — including a raw UUID that is untracked in state, which + // `resolveAssistantId` treats as "possibly deleted" and resolves to + // null. Sending the partial list would PATCH-replace `assistantIds` and + // wipe whatever assistant links are live on the dashboard; unlike tool + // destinations, there is no later pass that repairs a structured + // output's assistant links, so skip this one and warn instead. + const unresolved = authoredRefs.filter( + (ref) => !assistantRefIsTracked(ref, state), + ); + console.warn( + ` ⚠️ Structured output "${resourceId}" still references unresolved assistant(s): ${unresolved.join(", ")}. Leaving this structured output's assistant links untouched on the platform — they will link on a future push once those assistants exist.`, + ); + continue; + } + if (assistantIds.length > 0) { console.log(` 🔗 Linking structured output ${resourceId} to assistants`); const result = await vapiRequest("PATCH", `/structured-output/${uuid}`, { 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/so-assistant-omit.test.ts b/tests/so-assistant-omit.test.ts new file mode 100644 index 0000000..a8f74b9 --- /dev/null +++ b/tests/so-assistant-omit.test.ts @@ -0,0 +1,252 @@ +import assert from "node:assert/strict"; +import test from "node:test"; + +// ───────────────────────────────────────────────────────────────────────────── +// Structured outputs link to assistants via `assistant_ids` (snake_case, +// slugs), resolved to camelCase `assistantIds` (UUIDs) at push time. +// `resolveAssistantIds` (src/resolver.ts:103) silently drops any ref it can't +// resolve, so a structured output referencing an untracked assistant (a +// `--type structuredOutputs` push, say, or an assistant that simply hasn't +// been pushed yet) ends up with a shorter `assistantIds` array than what was +// authored. +// +// PATCH replaces the keys it receives, so sending that shorter array wipes +// whichever assistant links are already live on the dashboard: +// +// PATCH /structured-output/ { assistantIds: [] } // "resolved" nothing +// +// silently un-links every assistant the dashboard had, even though the file +// still authors every one of them. `omitUnresolvedAssistantIds` is the same +// omit-not-filter guard Task 1 added for tool destinations +// (`omitUnresolvedDestinations`), applied to the update path; the +// `updateStructuredOutputAssistantRefs` linking pass gets the analogous +// skip-and-warn instead of PATCHing a partial list. +// ───────────────────────────────────────────────────────────────────────────── + +process.argv = ["node", "test", "test-fixture-org"]; +process.env.VAPI_TOKEN = process.env.VAPI_TOKEN || "test-token-not-used"; + +const { omitUnresolvedAssistantIds, updateStructuredOutputAssistantRefs } = + await import("../src/push.ts"); + +import type { ResourceFile, StateFile } from "../src/types.ts"; + +const UUID = "8f14e45f-ceea-467a-9f1b-1a1b2c3d4e5f"; +const UUID_2 = "1a2b3c4d-5e6f-4a1b-9c2d-3e4f5a6b7c8d"; + +test("one unresolved ref among the authored assistant_ids drops the whole assistantIds key", () => { + const original = { + assistant_ids: ["front-desk", "clinical-stage-1"], + }; + const payload = { + name: "intake-schema", + assistantIds: [UUID], // only one of the two resolved + }; + + const result = omitUnresolvedAssistantIds(payload, original); + + assert.ok( + !("assistantIds" in result), + "the key must be absent so PATCH leaves the platform value untouched", + ); + assert.equal(result.name, "intake-schema", "the rest of the payload survives"); +}); + +test("a fully resolved assistantIds array is sent unchanged", () => { + const original = { assistant_ids: ["front-desk", "clinical-stage-1"] }; + const payload = { assistantIds: [UUID, UUID_2] }; + + const result = omitUnresolvedAssistantIds(payload, original); + + assert.deepEqual(result.assistantIds, [UUID, UUID_2]); +}); + +test("an authored ref with a trailing YAML comment still counts as one authored ref", () => { + // `assistant_ids: [front-desk ## front desk bot]` — the comment is part of + // the authored string, so the count has to strip it before comparing + // lengths, or a fully resolved single ref would look like a partial one. + const original = { assistant_ids: ["front-desk ## front desk bot"] }; + const payload = { assistantIds: [UUID] }; + + const result = omitUnresolvedAssistantIds(payload, original); + + assert.ok( + "assistantIds" in result, + "one authored ref, one resolved ref — nothing to omit", + ); +}); + +test("no assistant_ids on the original payload leaves the update untouched", () => { + const payload = { name: "intake-schema", assistantIds: [] }; + const result = omitUnresolvedAssistantIds(payload, {}); + assert.deepEqual(result, payload); +}); + +test("assistant_ids: [] on the original payload leaves the update untouched", () => { + const original = { assistant_ids: [] }; + const payload = { name: "intake-schema", assistantIds: [] }; + + const result = omitUnresolvedAssistantIds(payload, original); + + assert.deepEqual(result, payload); +}); + +// ───────────────────────────────────────────────────────────────────────────── +// `updateStructuredOutputAssistantRefs` runs after every other resource has +// applied — the linking pass, not the initial create. Before this change it +// PATCHed `resolveAssistantIds(...)` unconditionally whenever the result was +// non-empty, so a structured output with two authored refs where only one +// resolved would silently PATCH `assistantIds: []` and wipe the +// other link on the dashboard — with no warning at all, since the "all +// refs unresolved" case was the only one that skipped silently. Now any +// shortfall skips the PATCH and warns, naming which authored refs failed. +// ───────────────────────────────────────────────────────────────────────────── + +function emptyState(): StateFile { + return { + credentials: {}, + assistants: {}, + structuredOutputs: {}, + tools: {}, + squads: {}, + personalities: {}, + scenarios: {}, + simulations: {}, + simulationSuites: {}, + evals: {}, + }; +} + +async function withFetchAndWarnRecorders( + fn: (recorders: { + fetchCalls: unknown[]; + warnings: unknown[][]; + }) => Promise, +): Promise { + const fetchCalls: unknown[] = []; + const warnings: unknown[][] = []; + const originalFetch = globalThis.fetch; + const originalWarn = console.warn; + console.warn = (...args: unknown[]) => { + warnings.push(args); + }; + globalThis.fetch = (async (...args: unknown[]) => { + fetchCalls.push(args); + return { + ok: true, + status: 200, + json: async () => ({ id: UUID }), + text: async () => "{}", + } as unknown as Response; + }) as typeof globalThis.fetch; + + try { + return await fn({ fetchCalls, warnings }); + } finally { + globalThis.fetch = originalFetch; + console.warn = originalWarn; + } +} + +test("updateStructuredOutputAssistantRefs: skips the PATCH and warns when the single authored ref is absent from state", async () => { + const state = emptyState(); + state.structuredOutputs["intake-schema"] = { uuid: UUID }; + // Deliberately no entry under state.assistants for "front-desk" — the + // assistant is genuinely absent, not merely not-yet-applied. + + const so: ResourceFile = { + resourceId: "intake-schema", + filePath: "/fake/structured-outputs/intake-schema.yml", + data: { assistant_ids: ["front-desk"] }, + }; + + await withFetchAndWarnRecorders(async ({ fetchCalls, warnings }) => { + await updateStructuredOutputAssistantRefs([so], state); + + assert.equal( + fetchCalls.length, + 0, + "no PATCH should be sent when the assistant is genuinely absent", + ); + assert.ok( + warnings.some((args) => + args.some( + (arg) => + typeof arg === "string" && + arg.includes("intake-schema") && + arg.includes("front-desk"), + ), + ), + "a warning naming the structured output and the unresolved ref should be logged", + ); + }); +}); + +test("updateStructuredOutputAssistantRefs: a partial resolution (one of two) skips the PATCH and names only the failing ref", async () => { + const state = emptyState(); + state.structuredOutputs["intake-schema"] = { uuid: UUID }; + state.assistants["front-desk"] = { uuid: UUID_2 }; + // "clinical-stage-1" is deliberately untracked. + + const so: ResourceFile = { + resourceId: "intake-schema", + filePath: "/fake/structured-outputs/intake-schema.yml", + data: { assistant_ids: ["front-desk", "clinical-stage-1"] }, + }; + + await withFetchAndWarnRecorders(async ({ fetchCalls, warnings }) => { + await updateStructuredOutputAssistantRefs([so], state); + + assert.equal( + fetchCalls.length, + 0, + "sending the partial array would wipe the resolved link on the dashboard", + ); + assert.ok( + warnings.some((args) => + args.some( + (arg) => + typeof arg === "string" && + arg.includes("intake-schema") && + arg.includes("clinical-stage-1") && + !arg.includes("front-desk"), + ), + ), + // Checks that the resolved slug never appears in the warning at all — + // not just that it doesn't appear in one particular ordering. The + // implementation preserves authored order, so a buggy message naming + // both refs would read "front-desk, clinical-stage-1" (authored order) + // and slip past a check that only excluded "clinical-stage-1, front-desk". + "the warning should name the unresolved ref but not the resolved one", + ); + }); +}); + +test("updateStructuredOutputAssistantRefs: an untracked raw UUID counts as unresolved and skips the PATCH", async () => { + // A raw UUID that isn't in state resolves to null (resolveAssistantId + // treats it as "possibly deleted") — for structured outputs that must + // trigger the same skip-and-warn as an unresolved slug, since there is no + // later pass to repair a wiped link. + const state = emptyState(); + state.structuredOutputs["intake-schema"] = { uuid: UUID }; + + const so: ResourceFile = { + resourceId: "intake-schema", + filePath: "/fake/structured-outputs/intake-schema.yml", + data: { assistant_ids: [UUID_2] }, + }; + + await withFetchAndWarnRecorders(async ({ fetchCalls, warnings }) => { + await updateStructuredOutputAssistantRefs([so], state); + + assert.equal(fetchCalls.length, 0); + assert.ok( + warnings.some((args) => + args.some( + (arg) => typeof arg === "string" && arg.includes(UUID_2), + ), + ), + "the untracked UUID should be named in the warning", + ); + }); +}); diff --git a/tests/tool-assistant-cycle.test.ts b/tests/tool-assistant-cycle.test.ts new file mode 100644 index 0000000..c4fe8c2 --- /dev/null +++ b/tests/tool-assistant-cycle.test.ts @@ -0,0 +1,354 @@ +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 { + cleanDestinationAssistantIds, + omitUnresolvedDestinations, + unresolvedDestinationSlugs, + updateToolAssistantRefs, +} = await import("../src/push.ts"); + +import type { ResourceFile, StateFile } from "../src/types.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("an unresolved slug authored with a trailing YAML comment still drops the whole destinations key", async () => { + // Resolution failure leaves the assistantId exactly as authored — comment + // included — in the resolved payload, same as the original. The comparison + // must clean both sides before checking equality, or the commented, + // uncleaned resolved value never matches the cleaned original and the + // destination is wrongly classified as resolved. + const commented = "clinical-stage-1 ## stage one"; + const original = { + destinations: [{ type: "assistant", assistantId: commented }], + }; + const payload = { + destinations: [{ type: "assistant", assistantId: commented }], + }; + + const result = omitUnresolvedDestinations(payload, original); + + assert.ok( + !("destinations" in result), + "the key must be absent so PATCH leaves the platform value untouched", + ); +}); + +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); +}); + +// ───────────────────────────────────────────────────────────────────────────── +// `updateToolAssistantRefs` runs AFTER every other resource has already +// applied — it is the linking pass, not the initial create. Today it +// unconditionally PATCHes `resolved.destinations`, so a destination whose +// assistant is genuinely absent (not in state, not in the local repo) still +// carries a raw slug and the API answers `400 Assistant with ID "" not +// found`, aborting the whole push at the very end. `unresolvedDestinationSlugs` +// is the guard: it reports which destinations still carry a slug post- +// resolution so the linking pass can skip that tool and warn instead of +// PATCHing garbage. +// ───────────────────────────────────────────────────────────────────────────── + +test("unresolvedDestinationSlugs: a slug entry is reported", () => { + const result = unresolvedDestinationSlugs([ + { type: "assistant", assistantId: "clinical-stage-1" }, + ]); + assert.deepEqual(result, ["clinical-stage-1"]); +}); + +test("unresolvedDestinationSlugs: a trailing YAML comment is stripped before reporting", () => { + const result = unresolvedDestinationSlugs([ + { type: "assistant", assistantId: "clinical-stage-1 ## stage one" }, + ]); + assert.deepEqual(result, ["clinical-stage-1"]); +}); + +test("unresolvedDestinationSlugs: an array of only UUID assistantIds reports nothing", () => { + // Raw UUIDs are never "unresolved" here — the CREATE path already strips + // any destination whose resolved value equals the original, so a UUID that + // reaches this helper is either a genuinely resolved reference or an + // untracked-but-valid raw UUID the author wrote directly. Either way it must + // flow to the PATCH, not get reported as unresolved. + const result = unresolvedDestinationSlugs([ + { type: "assistant", assistantId: UUID }, + ]); + assert.deepEqual(result, []); +}); + +test("unresolvedDestinationSlugs: non-assistant destinations are not reported", () => { + const result = unresolvedDestinationSlugs([ + { type: "number", number: "+15550000000" }, + ]); + assert.deepEqual(result, []); +}); + +test("unresolvedDestinationSlugs: non-array input returns an empty list", () => { + assert.deepEqual(unresolvedDestinationSlugs(undefined), []); + assert.deepEqual(unresolvedDestinationSlugs(null), []); + assert.deepEqual(unresolvedDestinationSlugs("not-an-array"), []); +}); + +// ───────────────────────────────────────────────────────────────────────────── +// `cleanDestinationAssistantIds` is the last line of defense before the +// linking-pass PATCH body goes out: an untracked-but-valid raw UUID destination +// authored with a trailing `## comment` fails resolution (left exactly as +// authored) and passes `unresolvedDestinationSlugs` (which cleans before the +// UUID check), so without this the comment would reach the API and 400. +// ───────────────────────────────────────────────────────────────────────────── + +test("cleanDestinationAssistantIds: strips a trailing YAML comment from every destination's assistantId", () => { + const untrackedUuid = "1a2b3c4d-5e6f-7a8b-9c0d-1e2f3a4b5c6d"; + const result = cleanDestinationAssistantIds([ + { + type: "assistant", + assistantId: `${untrackedUuid} ## billing agent (unmanaged)`, + }, + { type: "number", number: "+15550000000" }, + ]); + + assert.deepEqual(result, [ + { type: "assistant", assistantId: untrackedUuid }, + { type: "number", number: "+15550000000" }, + ]); +}); + +test("cleanDestinationAssistantIds: a destination with no comment is a no-op", () => { + const result = cleanDestinationAssistantIds([ + { type: "assistant", assistantId: UUID }, + ]); + assert.deepEqual(result, [{ type: "assistant", assistantId: UUID }]); +}); + +test("cleanDestinationAssistantIds: non-array input is returned unchanged", () => { + assert.equal(cleanDestinationAssistantIds(undefined), undefined); + assert.equal(cleanDestinationAssistantIds(null), null); +}); + +function emptyState(): StateFile { + return { + credentials: {}, + assistants: {}, + structuredOutputs: {}, + tools: {}, + squads: {}, + personalities: {}, + scenarios: {}, + simulations: {}, + simulationSuites: {}, + evals: {}, + }; +} + +test("updateToolAssistantRefs: skips the PATCH and warns when the referenced assistant is genuinely absent", async () => { + const state = emptyState(); + state.tools["router"] = { uuid: UUID }; + // Deliberately no entry under state.assistants for "clinical-stage-1" — the + // assistant is genuinely absent, not merely not-yet-applied. + + const tool: ResourceFile = { + resourceId: "router", + filePath: "/fake/tools/router.yml", + data: { + type: "transferCall", + destinations: [{ type: "assistant", assistantId: "clinical-stage-1" }], + }, + }; + + const fetchCalls: unknown[] = []; + const originalFetch = globalThis.fetch; + const originalWarn = console.warn; + const warnings: unknown[][] = []; + console.warn = (...args: unknown[]) => { + warnings.push(args); + }; + globalThis.fetch = (async (...args: unknown[]) => { + fetchCalls.push(args); + return { + ok: true, + status: 200, + json: async () => ({ id: UUID }), + text: async () => "{}", + } as unknown as Response; + }) as typeof globalThis.fetch; + + try { + await updateToolAssistantRefs([tool], state); + + assert.equal( + fetchCalls.length, + 0, + "no PATCH should be sent when the assistant is genuinely absent", + ); + assert.ok( + warnings.some((args) => + args.some( + (arg) => + typeof arg === "string" && + arg.includes("router") && + arg.includes("clinical-stage-1"), + ), + ), + "a warning naming the tool and the unresolved slug should be logged", + ); + } finally { + globalThis.fetch = originalFetch; + console.warn = originalWarn; + } +}); + +test("updateToolAssistantRefs: PATCH body strips a trailing YAML comment from an untracked-but-valid raw UUID destination", async () => { + // The UUID is untracked (no state.assistants entry) but still a valid UUID + // shape, so it passes unresolvedDestinationSlugs and the linking pass + // proceeds to PATCH — the comment must not ride along in the request body. + const untrackedUuid = "1a2b3c4d-5e6f-7a8b-9c0d-1e2f3a4b5c6d"; + const state = emptyState(); + state.tools["router"] = { uuid: UUID }; + + const tool: ResourceFile = { + resourceId: "router", + filePath: "/fake/tools/router.yml", + data: { + type: "transferCall", + destinations: [ + { + type: "assistant", + assistantId: `${untrackedUuid} ## billing agent (unmanaged)`, + }, + ], + }, + }; + + const fetchCalls: unknown[] = []; + const originalFetch = globalThis.fetch; + globalThis.fetch = (async (...args: unknown[]) => { + fetchCalls.push(args); + return { + ok: true, + status: 200, + json: async () => ({ id: UUID }), + text: async () => "{}", + } as unknown as Response; + }) as typeof globalThis.fetch; + + try { + await updateToolAssistantRefs([tool], state); + + assert.equal(fetchCalls.length, 1, "the PATCH should still be sent"); + const [, init] = fetchCalls[0] as [string, RequestInit]; + const body = JSON.parse(init.body as string); + assert.deepEqual( + body.destinations, + [{ type: "assistant", assistantId: untrackedUuid }], + "the PATCH body's assistantId must be the bare UUID, no trailing comment", + ); + } finally { + globalThis.fetch = originalFetch; + } +});