Skip to content

fix[notask]: destructure embedding from new embed() return shape in sdk-tests#1598

Closed
donriddo wants to merge 1 commit into
tetherto:mainfrom
donriddo:chore/tests-qvac-embed-return-shape
Closed

fix[notask]: destructure embedding from new embed() return shape in sdk-tests#1598
donriddo wants to merge 1 commit into
tetherto:mainfrom
donriddo:chore/tests-qvac-embed-return-shape

Conversation

@donriddo

Copy link
Copy Markdown
Contributor

🎯 What problem does this PR solve?

📝 How does it solve it?

Destructure embedding from the new return shape in every call site that binds the result to a variable:

  • embedding-executor.ts — 2 sites (batch loop + single)
  • sharded-model-executor.ts — 2 sites (batch + per-text)
  • http-embedding-executor.ts — 1 site
  • error-executor.ts — 1 site (rename result → destructure embedding: result)

The 2 call sites that discard the return (error-executor.ts invalid-modelId test at line 44, logging-executor.ts fire-and-forget at line 56) are intentionally left as await embed(...) — they don't bind the result and just exercise the error/logging path.

🧪 How was it tested?

  • npm run typecheck — the embed-shape errors that existed pre-change are now resolved. Remaining typecheck errors (in delegated-inference-tests.ts and download-tests.ts about "function" literal type) are pre-existing and unrelated to this change; they also fail on main without this PR applied.

🔌 API Changes

None — this is a pure consumer migration to match the SDK's already-shipped embed return shape.

📋 Depends on

@qvac/sdk 0.9+ changes embed() to return `{ embedding, stats? }` instead
of raw vectors. Update the 6 `await embed(...)` call sites in sdk-tests
executors that bind the result to a variable so they destructure
`embedding` from the object. The 2 discard sites (error-executor invalid
modelId test, logging-executor fire-and-forget log) keep their existing
await-without-assignment form.
@donriddo

Copy link
Copy Markdown
Contributor Author

Superseded by #1596 — the sdk-tests fix is consolidated there alongside the CLI fix (both are missed-consumer fixes from #1495). Closing this one.

@donriddo donriddo closed this Apr 15, 2026
@github-actions

Copy link
Copy Markdown
Contributor

Tier-based Approval Status

**PR Tier:** TIER1

**Current Status:** ❌ PENDING

**Requirements:**
- 1 Team Member approval ❌ (0/1)
- 1 Team Lead OR Management approval ❌ (0/1)



---
*This comment is automatically updated when reviews change.*

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant