Skip to content

feat(compile): allow callers to disable embeddings - #169

Closed
TigerOfCountryYao wants to merge 1 commit into
atomicstrata:mainfrom
TigerOfCountryYao:codex/upstream-compile-disable-embeddings
Closed

feat(compile): allow callers to disable embeddings#169
TigerOfCountryYao wants to merge 1 commit into
atomicstrata:mainfrom
TigerOfCountryYao:codex/upstream-compile-disable-embeddings

Conversation

@TigerOfCountryYao

Copy link
Copy Markdown

Adds an optional embeddings flag to the compile API. The default remains true for full backward compatibility; callers that maintain an independent semantic index can explicitly pass false. Includes API and compile-path coverage. Verified with TypeScript, build, targeted tests, and changed-scope checks.

@ethanj

ethanj commented Aug 23, 2026

Copy link
Copy Markdown
Contributor

Sorry this sat so long without a look. Reviewing it as a draft since it is yours to finish, and the remaining work is small.

The feature is worth having, and your PR body undersells it. The description says "callers that maintain an independent semantic index", which invites the obvious objection: why not just withhold the embedding credential? The real answer is better than that, and it is provider-specific:

Provider Embedding credential Can a caller opt out today?
anthropic / claude-agent VOYAGE_API_KEY Yes, don't set it
openai OPENAI_EMBEDDINGS_API_KEY, then OPENAI_API_KEY No - the chat key doubles as the embedding key
ollama none required No - embeddings always run

So for two of the four backends there is no way to compile without embedding except to break chat. That is the case for the flag, and it belongs in the description, because "just unset the key" is the first thing a reviewer will reach for.

Scope is right too: safelyUpdateEmbeddings is the only embedding call reachable from compile, so gating it does stop all of the work, and you correctly note in the docs that the pending-embeddings drain goes with it.

Three things to finish.

1. The branch does not build. test/compile-options.test.ts is missing its closing }); - the describe block is never closed, so esbuild fails to transform the file and build-and-test has been red since July 30. That is almost certainly why this stalled. Everything below I found after adding that line locally.

2. The test survives the failure it exists to prevent. With the syntax fixed there is one case asserting expect(embedSpy).not.toHaveBeenCalled(). I mutation-tested it both ways:

Mutant Result
flag ignored, compile never embeds passes
flag ignored, compile always embeds fails

It proves embeddings: false does not embed, but it cannot tell that apart from embeddings being broken for everyone. Since the flag's whole promise is "the default is unchanged", the positive case is the more important half - assert the default path still calls the refresh, and the negative case becomes meaningful.

3. The finalizeWiki signature - already handled, you do not need to. That function gained a scoped parameter in the same position yours wants when #188 landed, so this now conflicts. Keeping both would leave two adjacent defaulted booleans, where transposing them type-checks silently and each one inverts a behaviour. That was our mess rather than yours, so I have put up #192 converting those to a named FinalizeFlags object. Once it lands, rebase and your flag becomes flags.embeddings alongside flags.scoped - named, and impossible to transpose.

One question rather than a request: there is no --no-embeddings on the CLI, while every other compile option (--review, --lang, --concurrency) is reachable from both surfaces. Deliberate, given the stated caller is programmatic? Either answer is fine, it just deserves a line.

Thanks for this, and again for the patience. The instinct is right and the gap is real.

ethanj added a commit that referenced this pull request Aug 23, 2026
`finalizeWiki` took `scoped` as a fifth positional boolean, added when the
prompt-modifier fingerprint landed (#188). #169 wants a sixth in the same
shape, to skip the embedding refresh.

Two adjacent defaulted booleans is the wrong place to be. They are the same
type, so transposing them type-checks silently, and each one INVERTS a
behaviour on a path whose whole point is to do less than the default: a scoped
run that must not record the modifier selection as true of the project, and a
compile that must not embed. Swapping them produces a compile that does the
opposite of what the caller asked, with nothing to catch it.

They move into a named `FinalizeFlags` object. One flag was tolerable; a second
one arriving is the signal to name them rather than to add a parameter. The
next flag is then `flags.x` at the call site and impossible to transpose.

No behaviour change. #188's scoped control was mutation-tested through the new
shape at both the call site and the destructure, and each mutant is still
caught.
@ethanj

ethanj commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

#192 has landed, so the signature is sorted and you are unblocked. finalizeWiki now takes a named FinalizeFlags object, so your change becomes flags.embeddings alongside flags.scoped rather than a sixth positional boolean.

That leaves the two things above: the missing }); in test/compile-options.test.ts, and a case asserting the default path still refreshes. No rush.

ethanj added a commit that referenced this pull request Aug 25, 2026
`compile({ embeddings: false })` runs page generation, links and the lexical index without any embedding-provider call or pending-embedding retry, for an SDK host that maintains its own semantic index. Omitting it is unchanged.

The gap it closes is provider-specific. With `anthropic` or `claude-agent` a caller can already opt out by not setting `VOYAGE_API_KEY`, but the `openai` embedding credential falls back to `OPENAI_API_KEY` and `ollama` needs no key at all, so those callers had no way to compile without embedding except to break chat.

The flag joins `FinalizeFlags` as `flags.embeddings` rather than becoming a sixth positional boolean beside `scoped`. It is deliberately NOT a prompt modifier: it changes what runs after pages are written, never what a prompt asks for, so it must not enter the modifier digest or invalidate a page that is byte-identical under it.

The original test asserted only that `embeddings: false` skips the refresh, which passes against a build that never refreshes at all. Two cases now pin the default path, and both mutants are caught: skipping unconditionally turns 2 red, ignoring the flag turns 1 red.

Based on #169 by @TigerOfCountryYao, whose commit is preserved in this branch's history.
@ethanj

ethanj commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

This shipped in #198, merged as 48fe1563.

Your commit is in the history as you wrote it, rebased onto current main rather than rewritten - git log and git blame both still show you as the author of those lines. The second commit on that branch is the test work.

Three things changed on the way in.

It builds now. test/compile-options.test.ts was missing its closing });, so esbuild could not transform the file and build-and-test had been red since the day this opened. That is almost certainly why it stalled, and it is worth knowing the fix was one line rather than anything about your design.

The default path is pinned. The original had one case, asserting that embeddings: false does not refresh. That is one-directional: it also passes against a build that never refreshes, so it cannot tell "the flag works" apart from "embeddings are broken for everyone". I confirmed by mutation - forcing the refresh to be skipped unconditionally left it green. Two cases now cover the default, and both mutants are caught.

It uses FinalizeFlags. Your version added a sixth positional boolean to finalizeWiki, next to scoped. Two same-typed flags in adjacent slots transpose silently, so #192 turned them into a named object first; yours went in as flags.embeddings. That conflict was ours, not yours - scoped landed after you wrote this.

One thing worth saying plainly: I argued against building this at first, because no issue requested it and it adds permanent public SDK surface. What changed my mind is the reason your description did not quite make. "Just don't set the embedding credential" only works for two of the four backends - openai falls back to OPENAI_API_KEY, which is the chat key, and ollama needs no key at all. Those callers had no way to compile without embedding except to break chat. That is the case for the flag, and it is worth making explicitly next time.

#171 is still open and is the more interesting of the three. I have been looking at it and will follow up there.

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.

2 participants